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

refactor: lower clickable style specificity by importing css module last #1508

Merged
merged 3 commits into from
Feb 24, 2023

Conversation

jinlee93
Copy link
Contributor

@jinlee93 jinlee93 commented Feb 24, 2023

Summary:

  • found out that css modules get loaded on demand, meaning if a component specific style sheet is imported before a subcomponent, the subcomponent style sheet will be imported into the html farther down than the component style sheet, causing specificity issues
    • we generally want the component styles to have priority over the styles of the consumed component
  • added lint rule to ensure css module style sheet is imported after the components
  • removed doubling of AccordionButton, Button, Menu Button class selectors in their css files
  • added :where pseudoselectors to ClickableStyle.module.css to keep specificity to (0,1,0) so they can be overriden via class styling
  • ran yarn lint --fix to push css module imports to the end of imports

Test Plan:

  • Wrote automated tests
    • new lint rule
  • CI tests pass
    • no visual regression, despite some styles specificity updates

@jinlee93 jinlee93 requested a review from a team February 24, 2023 09:26
@github-actions
Copy link

github-actions bot commented Feb 24, 2023

size-limit report 📦

Path Size
components 118.07 KB (-0.02% 🔽)
styles 3.1 KB (0%)

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #1508 (859574d) into next (efe9330) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             next    #1508   +/-   ##
=======================================
  Coverage   91.91%   91.91%           
=======================================
  Files         277      277           
  Lines        4178     4178           
  Branches      755      755           
=======================================
  Hits         3840     3840           
  Misses        313      313           
  Partials       25       25           
Impacted Files Coverage Δ
src/components/Accordion/Accordion.tsx 100.00% <100.00%> (ø)
src/components/AccordionButton/AccordionButton.tsx 96.77% <100.00%> (ø)
src/components/AccordionPanel/AccordionPanel.tsx 100.00% <100.00%> (ø)
src/components/AccordionRow/AccordionRow.tsx 100.00% <100.00%> (ø)
src/components/Badge/Badge.tsx 100.00% <100.00%> (ø)
src/components/Banner/Banner.stories.tsx 95.12% <100.00%> (ø)
src/components/Banner/Banner.tsx 100.00% <100.00%> (ø)
src/components/Breadcrumbs/Breadcrumbs.tsx 93.75% <100.00%> (ø)
src/components/BreadcrumbsItem/BreadcrumbsItem.tsx 100.00% <100.00%> (ø)
src/components/Button/Button.tsx 100.00% <100.00%> (ø)
... and 65 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +42 to +45
"pattern": "*.module.css",
"group": "index",
"patternOptions": { "matchBase": true },
"position": "after"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks for module.css and places after the index file, which is placed after imports from parents (where subcomponents live)

@@ -7,7 +7,7 @@
/**
* Accordion Button, wraps the heading and open indicator icon.
*/
.accordion-button.accordion-button {
.accordion-button {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removes doubling selectors in this file

*/
.button--primary.button--primary {
.button--primary {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removes doubling selectors in this file

@@ -24,7 +24,7 @@
transition-duration: var(--eds-anim-fade-quick);
transition-timing-function: var(--eds-anim-ease);

svg {
:where(svg) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added :where pseudo selectors to nested svg selectors to reduce specificity to (0, 1, 0) to be easily overridden by class selectors

@@ -61,7 +61,7 @@
/**
* Primary brand clickable style
*/
.clickable-style--primary.clickable-style--brand {
:where(.clickable-style--primary).clickable-style--brand {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced specificity to these double class selectors to (0,1,0) as well

@@ -19,7 +19,7 @@
width: 100%;
}

.menu__button.menu__button {
.menu__button {
Copy link
Contributor Author

@jinlee93 jinlee93 Feb 24, 2023

Choose a reason for hiding this comment

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

removed doubled/quadrupled class selectors in this file

@jinlee93
Copy link
Contributor Author

size-limit report 📦

Path Size
components 118.07 KB (-0.02% 🔽)
styles 3.1 KB (0%)

?? possibly with how bundling happens

@booc0mtaco
Copy link
Contributor

This is a great catch. I didn't realize we weren't consistent in this, but this follows the same order one would use if you managed the CSS (or any dependencies really) in the "classical" way:

  • for any component, import its dependencies first
  • include the code that may override or augment the functionality of the dependencies
  • implement the component, using the dependencies, and any overrides

@booc0mtaco
Copy link
Contributor

@jinlee93 would this be a good one to test as an alpha on GST? Logically, this will Just Work (tm)

@jinlee93
Copy link
Contributor Author

jinlee93 commented Feb 24, 2023

@jinlee93 would this be a good one to test as an alpha on GST? Logically, this will Just Work (tm)

Yea, TW classes have a lower specificity in GST than EDS due to the TW stylesheet being imported way earlier in the main app file and EDS components being imported ad hoc, so I imagine there wouldn't be any breaks where TW suddenly has more specificity over EDS components, but will alpha publish and test for sanity 👍

edit: might have spoken too soon https://github.com/chanzuckerberg/edu-stack/pull/32/commits/45912363535406a07d20d7e214fef08c8e5af12b#diff-4133eb55408b25b35b8e07c696a9dfc97b741c555d7407e8c86d0845e5eecc28R26-R28

Copy link
Contributor

@timzchang timzchang left a comment

Choose a reason for hiding this comment

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

Wow this is a really interesting learning! The change looks reasonable to me 👍🏼

@jinlee93
Copy link
Contributor Author

jinlee93 commented Feb 24, 2023

@booc0mtaco GST CI tests pass and Chromatic only has the InputField label changes with v9.2.0-alpha.0 published off of this branch

Copy link
Contributor

@booc0mtaco booc0mtaco left a comment

Choose a reason for hiding this comment

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

letsgo!

@jinlee93 jinlee93 merged commit 39d063f into next Feb 24, 2023
@jinlee93 jinlee93 deleted the jlee/lowerClickableStyleSpecificity branch February 24, 2023 18:45
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.

3 participants