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

feat(styles,carbon-react): switch to cds prefix #9719

Merged

Conversation

joshblack
Copy link
Contributor

Update our beta packages to use cds as the default prefix instead of bx. This PR includes two significant changes:

  • Update the default $prefix variable in @carbon/styles to be cds. It is still configurable under scss/_config.scss
  • Add a temporary src/prefix.js file in @carbon/react that changes the prefix in carbon-components. This will be removed in v11 and replaced by PrefixContext

Changelog

New

Changed

  • Switch default prefix from bx to cds for styles and React

Removed

Testing / Reviewing

  • View the deployed v11 storybook and verify that:
    • Styles are using the cds prefix
    • CSS Custom Properties are using the cds prefix
    • React components are using the cds prefix

@joshblack joshblack requested a review from a team as a code owner September 22, 2021 15:55
@netlify
Copy link

netlify bot commented Sep 22, 2021

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

🔨 Explore the source changes: 33b4672

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

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

@@ -92,6 +92,8 @@
},
"sideEffects": [
"es/feature-flags.js",
"lib/feature-flags.js"
"lib/feature-flags.js",
"es/prefix.js",
Copy link
Contributor

@abbeyhrt abbeyhrt Sep 22, 2021

Choose a reason for hiding this comment

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

Out of curiosity, why do these files (really all of them) have to be listed as sideEffects?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also curious. My guess is since it's pulled from carbon-components these should never be pruned when tree shaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abbeyhrt @tay1orjones basically these modules are not "pure" because they cause side effects in other modules (feature flags, components).

Bundlers like webpack/rollup will assume modules are pure by default if sideEffects: false is listed. This means that if exports from these entrypoints are used that the bundler will actually drop the file from the final output. This is not what we want because we always want these files to be included and their side effects run.

Annotating these files as having side effects ensures that they are included in the final bundle even though we aren't using exports from these files.

Hope this helps!

@netlify
Copy link

netlify bot commented Sep 22, 2021

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

🔨 Explore the source changes: 33b4672

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

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

@netlify
Copy link

netlify bot commented Sep 22, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 33b4672

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

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

Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

YAY!!! It's working! 🔥

@kodiakhq kodiakhq bot merged commit 1d7d399 into carbon-design-system:main Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants