Use math.div instead of slash for sass

[egriff38]

Closes #8746

This PR attempts to replace all instances of sass division by slash with math.div. First time contributor 😄

Changelog

Changed

• changes *.scss file slash division $var1 / $var2 over to math.div($var1, $var2)
• Added @use 'sass:math' where necessary, below imports.
• Changes one instance from dividing by 4 to multiplying by 0.25

Testing / Reviewing

• ✔️ yarn build passed
• ✔️ yarn lint passed
• 👎 yarn test fails, but only for the one line packages/layout/scss/_convert.scss#24. I suspect it may be an issue with node-sass?

[netlify]

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: fc2772b

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/612e63f3d4bf7d0008bf5e98

😎 Browse the preview: https://deploy-preview-9508--carbon-react-next.netlify.app

[github-actions]

DCO Assistant Lite bot All contributors have signed the DCO.

[netlify]

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: fc2772b

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/612e63f3bda69400070b7d2b

😎 Browse the preview: https://deploy-preview-9508--carbon-elements.netlify.app

[netlify]

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: fc2772b

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/612e63f3d3f58a0008be5dec

😎 Browse the preview: https://deploy-preview-9508--carbon-components-react.netlify.app

[egriff38]

I have read the DCO document and I hereby sign the DCO.

[joshblack]

Hi there @egriff38! 👋 Thanks so much for the first-time contribution! 🙏 We're so excited that you were able to take the time to make this PR.

I think currently @tay1orjones was taking a look at in in our sprint. I think our specific challenge is how to support both node-sass and sass users with these updates since, unfortunately, math.div is not available in node-sass.

One option for this could be that anytime we want to update a file to use math.div we could use the filename.import.scss trick, I'm curious if you think that could work with the changes that you made or if you think this already satisfies our node-sass requirements 👀

[egriff38]

Hi @joshblack, thanks for the welcome! I'm not familiar with the filename.import.scss trick. I do want to ask though why carbon continue to support node-sass when it was announced deprecated about 10 months back, and users opt in to their own sass compilers if they import the .scss files directly? Is there a significant portion of the user base still using node-sass over sass for compilation?

Besides the fix you mentioned, I'm unaware of any ways to make my change backwards compatible besides just leaving the files as they are and running the division migrator to maintain 2 separate versions (Though that would mean double the stylesheets to test...). I used the migrator for part of my PR and it worked much better than my find and replace to generate the changes.

[joshblack]

@egriff38 I think that's very fair to ask and will try my best to answer, let me know if there is anything else that I can help to clarify!

Is there a significant portion of the user base still using node-sass over sass for compilation?

Unfortunately yes, the majority of teams using Carbon are still using node-sass at the moment. We're trying to start transitioning people over to Dart Sass before v11 where we will make the switch officially over to Dart Sass.

This is where that *.import.scss idea came to mind to support @import through dart sass with the new math.div while still supporting node-sass which would resolve to *.scss. Reference: https://sass-lang.com/documentation/at-rules/import#import-only-files

[egriff38]

I think I understand what you mean. Do you think there could be some kind of polyfill module/import specifically for division that resolves to a / b for node-sass and math.div for dart-sass? Having a single utility polyfill file that gets imported everywhere could allow easier transition down the line to just @use sass:math. I need to better understand how the import-only files work though to know how to do that. We need to be sure the implementation is mutually exclusive across compilers so node-sass doesn't get a syntax error and dart-sass doesn't give deprecation warnings.

[tay1orjones]

@egriff38 Thanks for this PR! I took the liberty of riffing on this a bit to see if we could come up with something to satisfy the requirement of supporting node-sass and dart-sass.

• For the files that need to use math.div, I moved your changes into a new file, filename.import.scss. The existing files (filename.scss) I left as is.
• dart-sass will automatically pick up the *.import.scss files using math.div
• node-sass will automatically only use the normal filename.scss files
• I modified the style test to ensure that we don't try to build *.import.scss files with node-sass.

I avoided the idea of a polyfill because it's a bit of a logistical challenge. We'd need to have it available across these various different packages via a new package or copy/pasting the same polyfill in all the different packages. Using separate *.import.scss files will also make it easier to combat future problems we may run into between node-sass and dart-sass as the implementations diverge over time.

@joshblack The downside here is that we now have two implementations of the same file that we have to maintain. Worth noting that in v11 that will no longer be the case since we'll be using sass modules in @carbon/styles which aren't available through node-sass.

Let me know what you both think - I'm happy to field any questions and improve this further to meet the need.

[egriff38]

@tay1orjones, I definitely understand your reasoning on not using the polyfill, and your changes make sense to me; so long as dart-sass doesn't yell at me when I include the bundles I'll be happy. How soon do you anticipate v11 being published?

[tay1orjones]

I just pushed another update to clear up some deprecation warnings stemming from the carbon-react storybook.

so long as dart-sass doesn't yell at me when I include the bundles I'll be happy

@egriff38 Totally agree 😄 From my testing we no longer get any dart-sass division deprecation warnings in our own builds. Should have the same impact for you.

How soon do you anticipate v11 being published?

Full details and updated timelines can be found in the v11 Release issue. A beta is available and we're aiming to publish a second beta soon.

• v11 Release #7717
• [WIP] v11 Beta 2 #9322

[andreancardona]

@egriff38 @tay1orjones 🎉

[metonym]

For anyone running into the math.div is undefined error after upgrading, you must use sass version 1.33.0 or greater:

(node:73511) UnhandledPromiseRejectionWarning: Error: Undefined function.
return math.div($px, $carbon--base-font-size) * 1rem;

sass version 1.33.0 changelog