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(CSS pollution) #1671

Merged
merged 7 commits into from
Sep 25, 2023
Merged

Conversation

nstuyvesant
Copy link
Contributor

@nstuyvesant nstuyvesant commented Sep 25, 2023

Overview

  • Carbon Charts generated CSS with selectors not encapsulated within the top-level selector --cds--chart-holder. This caused the style sheet to overwrite CSS custom properties set by @carbon/styles and related packages.
  • Particularly problematic components were modal and the overflow-menu both used by the toolbar component. The imported CSS set CSS custom properties on root. These were refactored to avoid that issue.
  • Components grid-brush.scss and title only needed a limited number of SCSS variables and not generated classes. These now pull in the appropriate module where the variable is set to avoid generating unused CSS that conflicts with @carbon/styles.
  • Restructured toolbar into a folder with modules for modal and the toolbar buttons since this is interdependent. If someone doesn't need the toolbar, they can write an optimized SCSS script loading only what they need.
  • Closes [Bug]: When charts styles are imported after carbon styles, some components break #1669
  • Bumps Storybook, Eslint, @vitejs/plugin-react, SvelteKit and some @types within semver

Demo screenshot or recording

This StackBlitz example submitted with bug 1669 has the newly generated CSS pasted in place of the import:
https://stackblitz.com/edit/github-skkfec-q6ryuo?file=src%2Findex.scss

@nstuyvesant nstuyvesant requested review from theiliad, Akshat55 and a team as code owners September 25, 2023 02:21
@nstuyvesant nstuyvesant requested review from zvonimirfras and removed request for a team September 25, 2023 02:21
@netlify
Copy link

netlify bot commented Sep 25, 2023

Deploy Preview for carbon-charts-core ready!

Name Link
🔨 Latest commit 5d27087
🔍 Latest deploy log https://app.netlify.com/sites/carbon-charts-core/deploys/651178e1ba66f00008180888
😎 Deploy Preview https://deploy-preview-1671--carbon-charts-core.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 Sep 25, 2023

Deploy Preview for carbon-charts-angular ready!

Name Link
🔨 Latest commit 5d27087
🔍 Latest deploy log https://app.netlify.com/sites/carbon-charts-angular/deploys/651178e1b116cf000853b365
😎 Deploy Preview https://deploy-preview-1671--carbon-charts-angular.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 Sep 25, 2023

Deploy Preview for carbon-charts-react ready!

Name Link
🔨 Latest commit 5d27087
🔍 Latest deploy log https://app.netlify.com/sites/carbon-charts-react/deploys/651178e143c7d30008380a33
😎 Deploy Preview https://deploy-preview-1671--carbon-charts-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.

Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

LGTM

@theiliad theiliad changed the title Fix CSS pollution fix(CSS pollution) Sep 25, 2023
@theiliad theiliad merged commit 532b99e into carbon-design-system:master Sep 25, 2023
@theiliad theiliad deleted the grid-bg-fix branch September 25, 2023 15:15
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.

[Bug]: When charts styles are imported after carbon styles, some components break
2 participants