Make core colors classes and custom properties always available #31669
Conversation
oandregal
requested review from
mtias,
MaggieCabrera,
jasmussen,
scruffian and
youknowriad
oandregal
added
the
Global Styles
|
We can also make it so the core colors are not shown in the UI. Regarding both core and theme declaring the same color slug: what do we think of this? We can consolidate both into one (either keeping the name / value of core or theme), etc. Wanted to confirm direction before doing more work. |
|
Size Change: +236 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Love it. I think there's some design work in #27473 (comment) by @pablohoneyhoney that I think we can leverage here (see the theme/default/custom bits in the first screenshot). Or if not here, then in a followup PR. |
|
Added a before / after screenshot for the UI color panel, to have more context on the impact of this PR. I'm not sure we'll have time to update how the UI control works for 5.8 so I'm looking for low-hanging fruit that can be shipped. Is this, as it is, good for 5.8? An evolution of this PR I can think of include: do not show the core colors in the UI controls (they work as today: only show theme colors) but do enqueue core color classes and CSS Custom Properties. What do we think of both approaches? |
How does this affect the basic block editor interface, say this paragraph color panel:
If it does affect that, it's a bit more challenging as it adds a ton of colors to a UI that already has low compression, in which case I'd personally prefer the enqueuing of classes even if not surfacing of UI would be the preferable low hanging fruit solution. If it's mostly a global styles change, it's probably fine? Key to both is that we still consider and steer towards the design shown at the link, meaning we should avoid going too far down a path that takes us away from it. |
|
I just checked this on TT1 and emptytheme and I found out that the post editor is no longer showing colors for blocks. In the screenshot for the button block:
It shows the correct colors when I switch back to trunk. I was trying to find out what happens if the theme defines a color that already exists on the default palette (such as black and white) and if we should show the duplicates or not. |
Exactly, this change makes all colors show up in all the places we use the colors UI control: the block sidebar (both in post editor and site editor) as well as global styles sidebar (site editor).
Themes are not supposed to update core colors. However, if they provide the same slug as a core color (for example, both core and theme declare 'black') this is what happens with this PR:
|
|
I do worry about all colors showing up in all places with the current color picker UI we have. It just isn't good enough, we need something that doesn't explored in length like this does. It seems if we want a low hanging fruit option it would seem best to simply ship the CSS classes for now. Otherwise we need some sort of palette picker or collapsing interface to handle it. |
Nice. So my question would be: what is the expected result when some theme color slug is the same as the core's (see issue description)? Let's go with the example Maggie shared: both core and theme palettes provide a color with slug What |
|
What happens if black doesn't mean the same for the theme and the default palette? A theme may decide that black means |
The only bit the system uses to care about the "semantics" of the color is the With the current approach, the theme can re-write the meaning of
I can't repro 😢 It works for me fine with emptytheme, TT1-blocks, TT1. |
Looks like it only happens for the button block weirdly enough. The cover block, paragraph, pullquote... were showing the correct colors but not the buttons. |
I had to handle a similar case while working with the color annotation plugin and what we did was we checked if the value of the colors were the same, and if they were different we'd have them both, but we did rename them with a "custom" label. I wonder if it makes sense to have |
|
That's an excellent question to discuss and an important one to solve. Here are some paths forward with pros and cons: Core colors override theme colors
Theme colors override core colors
Core and theme colors coexist
Of the above three, while I appreciate that themes can integrate with patterns that leverage core colors, I can't help but feel like that is a bit of a hack. Perhaps we should reserve the slug of core colors and disallow themes using them? If we refine, polish and expand the core palette to compensate, that might still allow a wide range of expression. I keep coming back to the challenge outlined in this PR #30831 — how if you switch themes, a theme color you defined will go missing and |
If we go with the "theme color values override core color values" that's what we have. We do that dynamically with
body {
--wp--preset--color--black: #2D2D2D;
}
.has-black-color { color: #2D2D2D !important; }
.has-black-background-color { color: #2D2D2D !important; }
.has-black-border-color { color: #2D2D2D !important; }
body {
--wp--preset--color--black: #000000;
}
.has-black-color { color: #000000 !important; }
.has-black-background-color { color: #000000 !important; }
.has-black-border-color { color: #000000 !important; }So that's backed in this proposal. However, a different issue this PR doesn't touch is what happens when the color is use is a color theme, such as |
The issue with that approach is that classes are attached to the post content when the user selects a color from the preset. We can't rewrite existing posts, so, if the user did something that attached the class |
|
I tested the PR with Twenty Twenty-One and was able to reproduce the problem with the missing palette for the buttons. Twenty Twenty-One registers the slug "black", and when black is selected in the editor, both colors have checkmarks. I see the following JS warning: Windows 10, Chrome Version 90.0.4430.212, WP 5.7.1. |
|
Given the feedback here, I believe this needs to be reworked to not show the core colors in the UI control. There's the issue of what happens when the theme provides a color with the same slug as core's. Given that core colors will no be shown in the UI, I presume the behavior that makes more sense is that the theme's color overrides core's. |
oandregal
force-pushed
the
try/colors-are-not-merged
branch
from
b6ea487 to
a03cfc2
Compare
oandregal
force-pushed
the
try/colors-are-not-merged
branch
from
dc66b85 to
594e6c5
Compare
| @@ -6,124 +6,148 @@ | |||
| { | |||
| "name": "Black", | |||
| "slug": "black", | |||
| "color": "#000000" | |||
| "color": "#000000", | |||
| "origin": "core" | |||
There was a problem hiding this comment.
It seems redundant to have "origin": "core", in all of the colors. Here they are defined in the core theme.json so our code should automatically assume all these colors of core origin.
There was a problem hiding this comment.
Yeah, thought about that. These are the trade-offs I've considered:
- We only write this once and themes don't have to, it's something internal to core so it's transparent to the consumers of the API.
- If our code needs to augment that data, that needs to run for every request and makes the logic more complex.
- We don't know how the UI control that uses this data is going to work.
With these trade-offs, I leaned towards implementing the simplest thing that works and that can scale as our needs evolve.
Alternatives I've considered:
-
What if core colors (or colors not to show in the UI) were stored in a different part? Too complex to introduce at the stage we're in. We need to account for every level (top-level, blocks) and have access to both colors in the client for the case the theme doesn't provide any color.
-
What if we add a
showInUiflag instead oforigin? Will work the same, not super-opinionated. Although I thinkoriginis more future proof. -
What if we add the
originto all the colors? It's data we pass to the client that we don't need at the moment.
There was a problem hiding this comment.
Do we expect others to use this property? Something more descriptive like an array of which contexts to display the colors in UI may make sense.
My intuition would lean toward removing this property in the definition especially if we're unsure of usages by other consumers. Folks will copy paste examples, so I'd be a little careful here. We could do something like adding it in the in-memory representation or alternatively picking a different data structure like "core": { "settings": { "color": "pallete" : {. (I don't know this problem space very well, so just my quick takes, it may not make sense for this problem.)
There was a problem hiding this comment.
I've already shared my rationale about why I'd think going forward with the simplest approach that works is fine. However, one thing I didn't consider was the copy/paste rationale. That's a good point and makes me having second thoughts about this, to be honest.
| // Some others should be appended to the existing. | ||
| $to_append = array(); | ||
| $to_append[] = array( 'color', 'palette' ); | ||
| $to_append[] = array( 'color', 'gradients' ); |
There was a problem hiding this comment.
Already existing issue: It seems we need to include array( 'color', 'duotone' ) in either
to_append or to_replace.
There was a problem hiding this comment.
Did the same as the other color palettes: core is always enqueued and the theme is appended.
oandregal
force-pushed
the
try/colors-are-not-merged
branch
from
49dd85d to
172af8d
Compare
oandregal
force-pushed
the
try/colors-are-not-merged
branch
from
172af8d to
b7050eb
Compare
| @@ -103,13 +103,28 @@ function getPresetMetadataFromStyleProperty( styleProperty ) { | |||
| export const LINK_COLOR = '--wp--style--color--link'; | |||
| export const LINK_COLOR_DECLARATION = `a { color: var(${ LINK_COLOR }, #00e); }`; | |||
|
|
|||
| const filterColorsFromCoreOrigin = ( path, setting ) => { | |||
There was a problem hiding this comment.
Will this get out of sync with the implementation in block editor?
There was a problem hiding this comment.
I feel ultimately here, we'd need a way to get both
useSetting( 'color.palette' ) // return theme colors
useSetting( 'color.defaultPalette' ) // return default palette
and same for other settings. It's not important now, but we should make sure it's something we can achieve later.
| array( | ||
| 'slug' => 'green', | ||
| 'color' => 'green', | ||
| ), |
There was a problem hiding this comment.
Non-blocking, but it looks like we caught several cases with manual testing in the refactor rather than with tests. With that in mind are there any test cases that make sense to add?
I also wanted to note that as a reader with little context on the theme/color system, I don't know what to expect in behavior from a test case named "test_merge_incoming_data"
| @@ -6,124 +6,148 @@ | |||
| { | |||
| "name": "Black", | |||
| "slug": "black", | |||
| "color": "#000000" | |||
| "color": "#000000", | |||
| "origin": "core" | |||
There was a problem hiding this comment.
Do we expect others to use this property? Something more descriptive like an array of which contexts to display the colors in UI may make sense.
My intuition would lean toward removing this property in the definition especially if we're unsure of usages by other consumers. Folks will copy paste examples, so I'd be a little careful here. We could do something like adding it in the in-memory representation or alternatively picking a different data structure like "core": { "settings": { "color": "pallete" : {. (I don't know this problem space very well, so just my quick takes, it may not make sense for this problem.)
oandregal
force-pushed
the
try/colors-are-not-merged
branch
from
b7050eb to
81df19e
Compare
youknowriad
force-pushed
the
try/colors-are-not-merged
branch
from
81df19e to
d45d090
Compare
Implements first iteration of #29568
This PR makes it so the CSS styles (classes and custom properties) related to the core palette and gradients are always enqueued.
At the UI level, it doesn't change anything, and only the theme colors are shown.
Changes in this PR
useSettinghook to filter colors based on a key. If the color has the keyorigin: "core"it'll be filtered out, hence not used in the UI controls. Alternatives I've considered for this key are__experimentalNotUI. However, given that the final goal is to show them all at once, theoriginkey seems more future-proof.What happens if a theme uses the same slug as core
If a theme provides the same slug as a core color uses (for example, both core and theme declare
black) the theme's color classes and CSS Custom Properties are enqueued after the core's, so the theme's styles will override core's.At the UI level, there's no change: only the theme colors are shown.
How to test
Front-end:
#global-styles-inline-cssinline stylesheet has the corresponding CSS Custom Properties and classes for both core and theme colors.Post editor:
--wp--preset--color--purpleand.has-purple-colorto find the stylesheets.Site editor:
--wp--preset--color--purpleand.has-purple-colorto find the stylesheets.