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.

Sign up for GitHub

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

Jump to bottom

Components: Add stylelint rule for theme var regressions #58098

Merged
merged 6 commits into from
Jan 25, 2024
Merged

Components: Add stylelint rule for theme var regressions #58098

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2877a67
Components: Add stylelint rule for theme var regressions
mirka Jan 22, 2024
6037480
Disable rule for Button
mirka Jan 22, 2024
0d7acea
Fix in Guide
mirka Jan 22, 2024
1083948
Fix in Modal
mirka Jan 22, 2024
1f3e75a
Fix in Placeholder
mirka Jan 22, 2024
9fa84ec
Update changelog
mirka Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/block-library/src/embed/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,15 @@
.components-placeholder__error {
word-break: break-word;
}
}

.wp-block-embed__learn-more {
margin-top: 1em;

.components-placeholder__learn-more {
margin-top: 1em;
@at-root .wp-block-post-content & {
a {
color: var(--wp-admin-theme-color);
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/embed/embed-placeholder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const EmbedPlaceholder = ( {
{ _x( 'Embed', 'button label' ) }
</Button>
</form>
<div className="components-placeholder__learn-more">
<div className="wp-block-embed__learn-more">
<ExternalLink
href={ __(
'https://wordpress.org/documentation/article/embeds/'
Expand Down
23 changes: 23 additions & 0 deletions packages/components/.stylelintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/** @type {import('stylelint').Config} */
module.exports = {
extends: '../../.stylelintrc.json',
rules: {
'declaration-property-value-disallowed-list': [
{
'/.*/': '/--wp-admin-theme-/',
},
{
message:
'--wp-admin-theme-* variables do not support component theming. Use Sass variables from packages/components/src/utils/theme-variables.scss instead.',
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

},
],
},
overrides: [
{
files: [ './src/utils/theme-variables.scss' ],
rules: {
'declaration-property-value-disallowed-list': null,
},
},
],
};
2 changes: 2 additions & 0 deletions packages/components/src/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,13 @@

&:hover:not(:disabled, [aria-disabled="true"]) {
// TODO: Prepare for theming (https://github.com/WordPress/gutenberg/pull/45466/files#r1030872724)
/* stylelint-disable-next-line declaration-property-value-disallowed-list */
background: rgba(var(--wp-admin-theme-color--rgb), 0.04);
}

&:active:not(:disabled, [aria-disabled="true"]) {
// TODO: Prepare for theming (https://github.com/WordPress/gutenberg/pull/45466/files#r1030872724)
/* stylelint-disable-next-line declaration-property-value-disallowed-list */
background: rgba(var(--wp-admin-theme-color--rgb), 0.08);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/guide/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
}

li[aria-current="step"] .components-button {
color: var(--wp-components-color-accent, var(--wp-admin-theme-color));
color: $components-color-accent;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@
}

&.is-scrollable:focus-visible {
box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent;

// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 2px solid transparent;
Expand Down
5 changes: 0 additions & 5 deletions packages/components/src/placeholder/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,6 @@
padding: 0 $grid-unit-10 2px;
}
}
.components-placeholder__learn-more {
.components-external-link {
color: var(--wp-admin-theme-color);
}
}
Comment on lines -151 to -155
Copy link
Member Author

@mirka mirka Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

.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:

Embed block learn more link in correct editor UI color

Copy link
Contributor

@ciampo ciampo Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nice cleanup!

}


Expand Down
Loading