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): change rem function to to-rem #14420

Merged
merged 5 commits into from
Aug 11, 2023

Conversation

tw15egan
Copy link
Member

@tw15egan tw15egan commented Aug 10, 2023

Refs #14415
Refs sass/dart-sass#2059

Namespaces all rem functions to convert.to-rem used across the repo. Also, bumps sass to the latest version We'll bump this once the v10 fix is made so we can update incremental-migration example.This version bump was causing issues due to the change with rem upstream expecting two parameters https://github.com/sass/dart-sass/releases/tag/1.65.0

Changelog

Changed

  • rem function changed to to-rem
  • All @use './utilities/convert' as *; to @use './utilities/convert';
  • All instances of rem( have been changed to convert.to-rem(

Testing / Reviewing

Ensure there are no visual changes and all sass files compile successfully.

@netlify
Copy link

netlify bot commented Aug 10, 2023

Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit f3754dd
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/64d54f34ccd1fd000817b83f
😎 Deploy Preview https://deploy-preview-14420--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Aug 10, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit f3754dd
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/64d54f34c7b9e10008537622
😎 Deploy Preview https://deploy-preview-14420--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Could we also add some sassdoc comments where the rem function is declared, and then regenerate the sassdocs for the styles package? It might also be helpful if that comment lists the error message directly so searching the error returns those comments talking about how we have to use a namespace on rem()

Just trying to think about how we could try to prevent folks from asking about this in support, etc.

@nelsonchen90
Copy link
Contributor

Thanks, I hope this will be backport to v10 as well since the first ref issue is created for v10

@tw15egan
Copy link
Member Author

@nelsonchen90 yeah, we'll definitely get it backported as well since this will affect all users

@tw15egan tw15egan changed the title fix(@carbon/styles): scope rem namespace fix(@carbon/styles): change rem function to to-rem Aug 10, 2023
@tay1orjones tay1orjones added this pull request to the merge queue Aug 11, 2023
Merged via the queue into carbon-design-system:main with commit 8e78840 Aug 11, 2023
17 checks passed
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.

None yet

4 participants