Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to dart-sass #5216

Merged

Conversation

pgoldberg
Copy link
Contributor

@pgoldberg pgoldberg commented Apr 1, 2022

Fixes #4032, fixes #4751

Changes proposed in this pull request:

  • Migrate from node-sass to dart-sass
  • Run sass-migrator to replace / division operators with * or math.div() where appropriate

Reviewers should focus on:

No regressions

Screenshot

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @pgoldberg! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@adidahiya
Copy link
Contributor

Looks like you made some good progress here. I'm bumping stylelint & stylelint-config-palantir in #5217, so you should get lint passing after that merges.

@adidahiya adidahiya self-assigned this Apr 1, 2022
@adidahiya adidahiya changed the title Pgoldberg/run sass division migrator Migrate to dart-sass Apr 1, 2022
@adidahiya adidahiya marked this pull request as ready for review April 1, 2022 20:27
@adidahiya
Copy link
Contributor

New stylelint config has a bug, see palantir/stylelint-config-palantir#58. I've worked around it with a config override for now.

@adidahiya
Copy link
Contributor

preview builds: docs-app, demo-app

@adidahiya
Copy link
Contributor

There's a bug in menu items that have submenus, probably related to the @extend change. This text shouldn't have an underline:

image

also, the docs-wide search omnibar isn't highlighting results as expected (the JS functionality still works, but the styles have regressed):

2022-04-01 17 05 49

@pgoldberg pgoldberg force-pushed the pgoldberg/runSassDivisionMigrator branch 4 times, most recently from 7c963d3 to 38c7c91 Compare April 2, 2022 00:34
@pgoldberg
Copy link
Contributor Author

pgoldberg commented Apr 2, 2022

Fixed those in the last commit, although it's not nearly as nice as just extending the compound selectors

Tested with dark mode and intents (comparing against blueprintjs.com) to confirm it still works correctly in those circumstances

docs-app

image

fixed search

@pgoldberg pgoldberg force-pushed the pgoldberg/runSassDivisionMigrator branch 3 times, most recently from 2b4f912 to fb0642d Compare April 3, 2022 19:57
@pgoldberg pgoldberg force-pushed the pgoldberg/runSassDivisionMigrator branch from fb0642d to 9561284 Compare April 3, 2022 20:25
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, the menu item styles mixin implementation looks good 👍

@adidahiya adidahiya merged commit 8f25ff1 into palantir:develop Apr 4, 2022
@pgoldberg pgoldberg mentioned this pull request Apr 4, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants