Components: use new theming accent color in all components #45289
Conversation
|
Size Change: +1.78 kB (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
chad1008
force-pushed
the
refactor/component-theme-accent-variable
branch
from
664198e
to
719517a
Compare
|
Leaving this as a note for Future Me: On
With the original default blue this didn't really stand out, and even looked potentially intentional. With themes showing a different color though, it's a bit jarring. We'll probably want a followup to |
| @@ -27,16 +27,17 @@ $checkbox-input-size-sm: 24px; // Width & height for small viewports. | |||
| @include reduce-motion("transition"); | |||
|
|
|||
| &:focus { | |||
| box-shadow: 0 0 0 ($border-width * 2) $white, 0 0 0 ($border-width * 2 + $border-width-focus) var(--wp-admin-theme-color); | |||
| box-shadow: 0 0 0 ($border-width * 2) $white, 0 0 0 ($border-width * 2 + $border-width-focus) $components-color-accent; | |||
| border-color: $components-color-accent; | |||
There was a problem hiding this comment.
border-color was added because without it, the focused input box retained the default --wp-admin-theme-color border color:
| Before: | After: |
|
|
I wasn't able to identify the source of the clashing style, so it's either something coming from outside the package, or just something more experienced eyes will be able to locate more easily 🙂
There was a problem hiding this comment.
On second thought, as I continue working this doesn't feel like the right solution. I'm seeing other, similar border issues (e.g. on FormTokenField).
They look suspiciously like the focus styles in utils/input/base.js (which will be updated during the course of this PR), but updating the vars in that file doesn't seem to have an impact. Once I sort out what's actually going on, this added border-color declaration will probably be removed.
There was a problem hiding this comment.
After some guidance from @ciampo, was able to identify the @wordpress/base-styles mixins that were at the root of these colors. I've also added &[aria-checked="mixed"] to the CheckboxControl's styles, as that was also using the --wp-admin-theme-color in the mixin.
There was a problem hiding this comment.
@mirka this may be a recurring theme (pun not intended) — some styles may come from the base-styles package, and we'll have to decide if we should override them in the components package with our own theme variable, or if we should just leave them as they are.
There was a problem hiding this comment.
There were a few more of these as well. I've added comments and/or isolated those changes in separate commits so they're easy to remove if we decide to leave them alone for the time being.
There was a problem hiding this comment.
Interesting! I played around with how CSS variable reassignment works to see if it would suffice for our use case, but unfortunately it all becomes circular so it won't work 😂
.component-foo {
// It would've been nice if the var() was evaluated before assignment, but it doesn't work like that 🙃
--wp-admin-theme-color: var(--wp-admin-theme-color, somefallback);
@include some-base-styles-mixin;
}I am actually now considering decoupling wp-components from some of the base-styles mixins (i.e. copy/paste the relevant stuff over). The mixin dependencies were already hard to work with in wp-components, not to mention being a blocker for Emotion migration. In this CheckboxControl for example, you can see some of the styles are duplicated for some/no reason. And the fact that WP's "canonical" CheckboxControl styles have already diverged from the base-styles mixin means that the mixin isn't useful for style consistency anyway 🤷
tl;dr — For this PR I am fine with leaving the base-styles overrides out, so we can address them in a coordinated way later.
There was a problem hiding this comment.
I am actually now considering decoupling wp-components from some of the base-styles mixins
That's my gut feeling too. On one hand, those mixins have the potential to provide consistent styles throughout the whole Gutenberg repo — but on the other hand, they are written in a way that makes them too leaky and hard to work with as a "loose" dependency.
If the net result is that today we have to write many overrides for those mixins anyway, I agree that we should decouple them from wpcomponents, and move over the relevant stuff in a way that works better for the package
There was a problem hiding this comment.
tl;dr — For this PR I am fine with leaving the base-styles overrides out, so we can address them in a coordinated way later.
Should we have a tracking issue where we list the overrides? Or do you think that the problem is so widespread that it's a wasted effort?
There was a problem hiding this comment.
Should we have a tracking issue where we list the overrides? Or do you think that the problem is so widespread that it's a wasted effort?
If it's useful for task coordination, sure. I think it's mostly just @include input-control.
There was a problem hiding this comment.
Cool. I also proposed it since Chad mentioned that there are a few instances where he had to add such overrides:
I've added comments and/or isolated those changes in separate commits so they're easy to remove if we decide to leave them alone for the time being.
chad1008
force-pushed
the
refactor/component-theme-accent-variable
branch
from
fdbb07a
to
0a2cd52
Compare
|
|
||
| &:focus { | ||
| border-color: $components-color-accent; | ||
| box-shadow: 0 0 0 ($border-width-focus - $border-width) $components-color-accent; |
There was a problem hiding this comment.
border-color and box-shadow were previously defined by @wordpress/base-styles for both .is-active and :focus. For now, overrides have been added to the component styles to allow theming to take effect.
| @@ -3,7 +3,7 @@ | |||
| font-family: $default-font; | |||
| font-size: $default-font-size; | |||
| background-color: $white; | |||
| border-left: 4px solid var(--wp-admin-theme-color); | |||
| border-left: 4px solid $components-color-accent; | |||
There was a problem hiding this comment.
This is an interesting case for theming... on some themes (like the "Sunset" theme we're currently using in Storybook) the notices can take on an orange color. With another theme it could even be red. These colors on notices usually have more meaning than they would in other components, indicating the severity/urgency of the notice.
Not sure if/how that impacts how we'd want to apply theming on this component.
There was a problem hiding this comment.
That's an interesting point. I will note that the success, warning and error statues of notice already have a hardcoded border color (a few lines below in this file), and so this specific rule would only affect the "default" and
"informational" statuses.
Current behaviour is also explained in the README — we should keep that in mind in case it needs updating too
There was a problem hiding this comment.
Current behaviour is also explained in the README — we should keep that in mind in case it needs updating too
Good call. I've added an update to the README ✅
| @@ -18,7 +18,8 @@ | |||
|
|
|||
| &:focus { | |||
| background: $white; | |||
| 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; | |||
| border-color: $components-color-accent; | |||
There was a problem hiding this comment.
border-color was previously defined by @wordpress/base-styles for:focus. For now, an override has been added to the component styles to allow theming to take effect.
| @@ -148,7 +148,7 @@ export const DropdownMenu = css` | |||
| `; | |||
|
|
|||
| export const ResetLabel = styled.span` | |||
| color: var( --wp-admin-theme-color-darker-10 ); | |||
| color: ${ COLORS.ui.borderFocus }; | |||
There was a problem hiding this comment.
Along with this change, there are a couple of other pre-existing places in the codebase where this variable is being used for things other than a border... do we want to consider renaming it in utils/color-values.js in a followup PR?
There was a problem hiding this comment.
We haven't yet audited these "semantic" color variables yet, so I don't know whether any of them are still relevant. The semantics are potentially useful when theming so we should keep them around until we audit them. That also means I don't want to force any new usages into these existing semantics if it doesn't make sense.
This ResetLabel isn't related to a borderFocus, so I would consider exposing a COLORS.ui.themeDark10 that we can use here. (Aaand it looks like you already did that! 😄)
Also unclear why this ResetLabel is a darker-10 and not just the theme color 🤔 Do you remember, @aaronrobertshaw?
There was a problem hiding this comment.
Also unclear why this ResetLabel is a darker-10 and not just the theme color 🤔 Do you remember
Thanks for the ping.
I'm drawing a blank on why that CSS var was actually used.
As long as the color of the ResetLabel matches the "Saved" in the top toolbar when saving a draft post it will satisfy the request here.
Further context and history can be found in #44260.
There was a problem hiding this comment.
Was looking at this with @mirka a little and this button doesn't have a disabled state, the 'reset' just isn't displayed when it isn't an option.
Looking at the focused/hovered state, I think I'd recommend we keep the current darker color. If the Reset span is given the same accent color as the rest of the list item, it becomes difficult to differentiate which button you're actually interacting with:
Screen.Recording.2022-11-03.at.11.36.59.mov
With the current darker color, there's at least a little contrast creating some distinction, though it is really subtle. We may want something with more contrast/decoration, but probably not something for this PR.
Any objection to keeping the slightly darker reset for now, using the newly exposed variable?
There was a problem hiding this comment.
Yeah, we can keep it as is 👍 Thanks for looking into it.
|
Looking at the current unit test failure, I think we need to update a snapshot in the |
chad1008
force-pushed
the
refactor/component-theme-accent-variable
branch
from
f41a035
to
8d2f12c
Compare
Yep, let's update snapshots as part of this PR |
chad1008
force-pushed
the
refactor/component-theme-accent-variable
branch
from
7167a73
to
afbde74
Compare
Done. Rebased the PR, waiting to see what CI thinks of it now! |
| @@ -148,7 +148,7 @@ export const DropdownMenu = css` | |||
| `; | |||
|
|
|||
| export const ResetLabel = styled.span` | |||
| color: var( --wp-admin-theme-color-darker-10 ); | |||
| color: ${ COLORS.ui.borderFocus }; | |||
There was a problem hiding this comment.
We haven't yet audited these "semantic" color variables yet, so I don't know whether any of them are still relevant. The semantics are potentially useful when theming so we should keep them around until we audit them. That also means I don't want to force any new usages into these existing semantics if it doesn't make sense.
This ResetLabel isn't related to a borderFocus, so I would consider exposing a COLORS.ui.themeDark10 that we can use here. (Aaand it looks like you already did that! 😄)
Also unclear why this ResetLabel is a darker-10 and not just the theme color 🤔 Do you remember, @aaronrobertshaw?
|
|
||
| // Override for theming on the block mover handle. `.block-editor-block-mover__drag-handle` was needed for specificity to override declaration order. | ||
| .components-accessible-toolbar &.block-editor-block-mover__drag-handle:focus::before { | ||
| box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent, inset 0 0 0 4px $white; | ||
| } |
There was a problem hiding this comment.
I am very reluctant to let block-editor taint this file again because Marco just recently cleaned it up 😄
I'm thinking we should leave all the non-components-package stuff for later so we can address it in a coordinated way.
There was a problem hiding this comment.
I'm thinking we should leave all the non-components-package stuff for later so we can address it in a coordinated way.
Do you mean all overrides, including the ones related to the base styles? Or just the overrides needed for other packages too (i.e block editor) ?
There was a problem hiding this comment.
In this comment I was referring to the latter (other packages like block-editor). But yeah, I think base-styles is better addressed separately as well.
The basic principle I'm trying to stick with is that Theme is a wp-components concept, and wp-components is the upstream dependency, so if downstream packages like block-editor want to participate they need to use whatever API that wp-components has provided. What that API should look like is to be decided, but we'll probably need to export $components-color-accent, COLORS.ui.theme, etc from wp-components.
There was a problem hiding this comment.
In this comment I was referring to the latter (other packages like block-editor). But yeah, I think base-styles is better addressed separately as well.
The basic principle I'm trying to stick with is that Theme is a wp-components concept, and wp-components is the upstream dependency, so if downstream packages like block-editor want to participate they need to use whatever API that wp-components has provided.
Agreed on both points.
What that API should look like is to be decided, but we'll probably need to export
$components-color-accent,COLORS.ui.theme, etc from wp-components.
We'll have to be careful in doing this, since we won't be able to undo these API decisions (because of WordPress backwards compat rules).
I also wonder what is the better format to expose those theme variables — as Sass + Emotion vars, as CSS vars.
| @@ -27,16 +27,17 @@ $checkbox-input-size-sm: 24px; // Width & height for small viewports. | |||
| @include reduce-motion("transition"); | |||
|
|
|||
| &:focus { | |||
| box-shadow: 0 0 0 ($border-width * 2) $white, 0 0 0 ($border-width * 2 + $border-width-focus) var(--wp-admin-theme-color); | |||
| box-shadow: 0 0 0 ($border-width * 2) $white, 0 0 0 ($border-width * 2 + $border-width-focus) $components-color-accent; | |||
| border-color: $components-color-accent; | |||
There was a problem hiding this comment.
Interesting! I played around with how CSS variable reassignment works to see if it would suffice for our use case, but unfortunately it all becomes circular so it won't work 😂
.component-foo {
// It would've been nice if the var() was evaluated before assignment, but it doesn't work like that 🙃
--wp-admin-theme-color: var(--wp-admin-theme-color, somefallback);
@include some-base-styles-mixin;
}I am actually now considering decoupling wp-components from some of the base-styles mixins (i.e. copy/paste the relevant stuff over). The mixin dependencies were already hard to work with in wp-components, not to mention being a blocker for Emotion migration. In this CheckboxControl for example, you can see some of the styles are duplicated for some/no reason. And the fact that WP's "canonical" CheckboxControl styles have already diverged from the base-styles mixin means that the mixin isn't useful for style consistency anyway 🤷
tl;dr — For this PR I am fine with leaving the base-styles overrides out, so we can address them in a coordinated way later.
|
per @mirka's advice above, I've reverted all of the
I hadn't actually carried that all the way into the exported
|
|
Sorry for the extra ping @aaronrobertshaw - missed your reply above before! |
|
That's nice! I had to override some editor styles because the editor UI uses the secondary color (which isn't always intended), this would also be changed by this PR? |
@strarsis Hmm, I'm not sure what you mean by "secondary color" in this context. Could you open a bug report if you're still seeing the issue? If any UI colors are wrong, we'd like to fix them 🙂 |
closes #45249
part of #44116
What?
Updates the Components package to use the new accent color variables in all relevant components.
Why
Because colors look nice.
How?
First updating the emotion variables to use the new component theme var, with the proper fallbacks.
Then updating any references to the existing WP theme variables to instead point to the new component theme variables in individual component styles.
TODO List
utils/input/base.js)For each of the above, we'll need to:
<Theme>componentThemecomponent differentaccentprops to test the color changeTesting Instructions
Additional Notes
Some things that came up, but don't necessarily fit in this PR. Some of these are mentioned in comments below, but I wanted to gather them in a single location:
AngleCircleofAnglePickerControl. This outline is immune to theming, but we should be able to fix that easily. Question is do we want that outline showing? Or do we want to hide it?Noticestyling. We may need to updateNotice'sReadme.base-stylespackage need to be overridden for theming to take effect. On this PR, all such overrides are commented and separated into their own commits in case we decide not to override them.COLORS.ui.borderFocusedis mapped to thedarker-10color variant and as of this PR, our new$components-color-accent-darker-10variable. ThatborderFocusedproperty isn't used exclusively for borders though. Should it be renamed?Inserter(ablock-librarycomponent) has SVGs icons for the various blocks that do not respect our theming. I don't think a simple CSS override will do here? 🤔block-editor'sURLPopover,TextDecorationControl, andTextTransformControldon't currently respect theming. We can probably override those styles without too much trouble, but I didn't want to hold this PR up while I dove down that rabbit hole. If desired, I can happily work to add those changes here 🙂