Components: Add stylelint rule for theme var regressions #58098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
added
[Type] Code Quality
23 tasks
mirka
commented
Jan 22, 2024
Comment on lines
-151
to
-155
| .components-placeholder__learn-more { | ||
| .components-external-link { | ||
| color: var(--wp-admin-theme-color); | ||
| } | ||
| } |
There was a problem hiding this comment.
.components-placeholder__learn-more is only used in the Embed block in wp-block-library, so it doesn't belong here. Moved and renamed.
For reference, this style was correcting a style override in the editor writing flow where it was showing this "Learn more" link in the Embed block with the site content link color, rather than the editor UI color:
|
Size Change: +222 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1f3e75a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7618886355
|
ciampo
reviewed
Jan 23, 2024
| }, | ||
| { | ||
| message: | ||
| '--wp-admin-theme-* variables do not support component theming. Use Sass variables from packages/components/src/utils/theme-variables.scss instead.', |
There was a problem hiding this comment.
What about --wp-components-color-* variables? We've been using them in CSS-in-JS-styled components, for example (where SASS variables are not an option)
There was a problem hiding this comment.
Good question. We can address this separately and incrementally after #58097 is merged so we have variables built into the COLORS object. The linting considerations for --wp-components-color-* are much more complicated than --wp-admin-theme-*, so I'd like to think through that better. Fortunately it's very easy to notice when --wp-components-color-* is used without fallbacks, so direct usage is unlikely to cause visual regressions, just a matter of code DRYness.
Interestingly, I found usages of --wp-components-color-* in dataviews and edit-site 😅 I'll address that separately as well.
There was a problem hiding this comment.
Makes sense.
For next steps, what we could do, at least at a high level, could be:
- to reword the warning message from "Use Sass variables instead" to "Use Sass variables or the
--wp-components-*CSS variables (with a fallback) instead" - add more linting rules:
- in components package, to make sure that
--wp-components-*CSS variables are always associated with a fallback value - outside of components package, we should probably prohibit the usage of
--wp-components-*CSS variables altogether
- in components package, to make sure that
What do you think?
There was a problem hiding this comment.
Interestingly, I found usages of --wp-components-color-* in dataviews and edit-site 😅 I'll address that separately as well.
Ouch 😅 Yup, should definitely be addressed. Another confirmation that those linter rules could be beneficial!
There was a problem hiding this comment.
For next steps, what we could do, at least at a high level
I think there are no cases (at least in SCSS files) where --wp-components-* vars need to be used instead of the Sass vars (which already have the correct fallbacks built in), so it's likely better to enforce the Sass var usage.
Outside the components package, there are possible use cases that are legitimate, but we aren't ready to commit to anything yet so a blanket restriction is probably the way to go for now. (And the existing usages don't seem necessary.)
This was referenced Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #44116
What?
Adds a Stylelint rule for stylesheets under
packages/componentsto prevent unknowing usage of--wp-admin-theme-*color variables.Why?
--wp-admin-theme-*variables are coupled with wp-admin and do not support our component theming system.There were already three new occurrences of the variable since we removed them all, and that excludes the ones we caught in review. So I think it's worth setting up a lint rule.
I'll set up a separate rule for Eslint so we can also catch them in JS files (#58130).
Testing Instructions
2877a67that adds the stylelint rule, and runnpm run lint:css. It should error on the offending files underpackages/components.