Components: use new theming accent color in all components #45289
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,6 @@ | |
| width: 100%; | ||
|
|
||
| &.is-selected { | ||
| box-shadow: 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -27,16 +27,18 @@ $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) $components-color-accent; | ||||||
| border-color: $components-color-accent; | ||||||
|
There was a problem hiding this comment.
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 They look suspiciously like the There was a problem hiding this comment. After some guidance from @ciampo, was able to identify the There was a problem hiding this comment. @mirka this may be a recurring theme (pun not intended) — some styles may come from the 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.
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.
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.
If it's useful for task coordination, sure. I think it's mostly just 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:
|
||||||
|
|
||||||
| // Only visible in Windows High Contrast mode. | ||||||
| outline: 2px solid transparent; | ||||||
| } | ||||||
|
|
||||||
| &:checked, | ||||||
| &:indeterminate, | ||||||
| &[aria-checked="mixed"] { | ||||||
| background: $components-color-accent; | ||||||
| border-color: $components-color-accent; | ||||||
|
|
||||||
| // Hide default checkbox styles in IE. | ||||||
| &::-ms-check { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| font-family: $default-font; | ||
| font-size: $default-font-size; | ||
| background-color: $white; | ||
| 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 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.
Good call. I've added an update to the README ✅ |
||
| margin: 5px 15px 2px; | ||
| padding: 8px 12px; | ||
| align-items: center; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,7 +148,7 @@ export const DropdownMenu = css` | |
| `; | ||
|
|
||
| export const ResetLabel = styled.span` | ||
| 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 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 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.
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.movWith 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. |
||
| font-size: 11px; | ||
| font-weight: 500; | ||
| line-height: 1.4; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very reluctant to let
block-editortaint 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Themeis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on both points.
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.