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

fix(@carbon/styles): remove / operator, use math.div #9078

Closed

Conversation

tw15egan
Copy link
Collaborator

Closes #8746

WIP, will update as more components are added to the @carbon/styles package

Changelog

Changed

  • Replace / symbol with the math.div function or * where possible

Testing / Reviewing

Ensure styles remain the same with the changes and no deprecation warnings are thrown

@netlify
Copy link

netlify bot commented Jun 30, 2021

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

🔨 Explore the source changes: b4a51da

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

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

@netlify
Copy link

netlify bot commented Jun 30, 2021

❌ Deploy Preview for carbon-elements failed.

🔨 Explore the source changes: b4a51da

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

@netlify
Copy link

netlify bot commented Jun 30, 2021

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

🔨 Explore the source changes: b4a51da

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

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

@tw15egan
Copy link
Collaborator Author

@joshblack seems like this is causing all the tests to fail in @carbon/styles since the test renderer is using node-sass under the hood. Any ideas?

@joshblack
Copy link
Contributor

@tw15egan hmm, if it's using SassRenderer for the tests it should be picking up sass 🤔 I wonder if our version is too old or something for the new math function?

As a side-note, would it make sense to start with these changes over in packages/components without math.div (basically just the multiplication change) and then update @carbon/styles to use math.div where multiplication doesn't make sense?

This would end up helping out teams who are making the move to dart sass now so they don't have to see warnings in their console all over the place.

@tw15egan
Copy link
Collaborator Author

tw15egan commented Jul 7, 2021

It looks like there are only a few places where we use the / sign directly, but I'll update those in this PR. I'm guessing the bulk of the warnings are coming from the rem function (@return ($px / $base-font-size) * 1rem;) that will need to be replaced with math.div

@tw15egan
Copy link
Collaborator Author

tw15egan commented Jul 7, 2021

Seems like the @carbon/grid package is another big one that needs to be updated, as it is using / quite a bit in its calculations
Screen Shot 2021-07-07 at 11 48 00 AM

@tw15egan tw15egan closed this Aug 3, 2021
@tw15egan tw15egan deleted the remove-division-symbol branch March 3, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update usage of / operator to use math.div
2 participants