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

RSTUDIO-538: Switch to the sass library #1654

Merged
merged 11 commits into from
Jun 7, 2024
Merged

Conversation

gagik
Copy link
Collaborator

@gagik gagik commented Jun 6, 2024

Changes the deprecated node-sass dependency and uses https://github.com/sass/migrator#readme to mitigate warnings from the migration, namely: https://sass-lang.com/documentation/breaking-changes/slash-div/

Resolves #1652

@cla-bot cla-bot bot added the cla: yes label Jun 6, 2024
@gagik gagik changed the base branch from channel/major-15 to gagik/update-realm-version June 6, 2024 13:35
@gagik gagik marked this pull request as ready for review June 6, 2024 13:46
@@ -70,7 +70,7 @@
top: -$spacer;

&__Btn {
margin: 0 ($spacer / 4);
margin: 0 (calc($spacer / 4));
Copy link
Member

Choose a reason for hiding this comment

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

Using calc makes this compute when evaluating the CSS rather than when compiling the SASS and adds verbosity. What's the context on these changes?

Copy link
Collaborator Author

@gagik gagik Jun 6, 2024

Choose a reason for hiding this comment

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

It's related to this: https://sass-lang.com/documentation/breaking-changes/slash-div/
math.div may be a more equivalent change, it's just required imports.
Actually this let me notice that they have an automatic migrator, so I could run that and see what the output is

Copy link
Collaborator Author

@gagik gagik Jun 6, 2024

Choose a reason for hiding this comment

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

sass does actually simplify calc expressions to a single number in most cases as well although it's a bit different from plain /

@gagik gagik force-pushed the gagik/update-sass branch from 36aa8b8 to 3ddcb22 Compare June 6, 2024 14:20
@@ -70,7 +70,7 @@
top: -$spacer;

&__Btn {
margin: 0 ($spacer / 4);
margin: 0 ($spacer * 0.25);
Copy link
Collaborator Author

@gagik gagik Jun 6, 2024

Choose a reason for hiding this comment

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

Interestingly enough, this is what the sass migrator tool does for this: https://github.com/sass/migrator#readme. Seems like the way to go

@kraenhansen It is a lot less verbose now, what do you think?

Base automatically changed from gagik/update-realm-version to channel/major-15 June 7, 2024 08:02
@gagik gagik merged commit 215ea6b into channel/major-15 Jun 7, 2024
3 checks passed
@gagik gagik deleted the gagik/update-sass branch June 7, 2024 08:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the new sass library instead of node-sass
3 participants