Enqueue core and theme colors by using separate structures per origin. #32358
Conversation
jorgefilipecosta
added
the
Global Styles
jorgefilipecosta
requested review from
ellatrix,
oandregal and
spacedmonkey
as code owners
|
Size Change: +106 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
Thanks for trying this PR @jorgefilipecosta Where I think I have a different opinion than you is that I think from the That said, I do think the "default color palette" or the "default presets" are just other settings. In other words:
What do you think? |
|
Hi @youknowriad,
Exactly that's what this PR does, for normal settings there is no source separation.
That is also the approach this PR is doing with the exception we call "core" and not "default" but I will change. I think this approach is doing what you are describing we just have a settings object and for presets that object contains subobjects each one defining the preset source. So this PR contains a single settings object, every setting excluding the four presets continues exactly as is. For presets instead of color.palette being an array of colors it is an object where each property "core", "theme", and "user" is an array of colors coming from a given source. Is there anything I'm missing that you think we should change? |
|
Actually, it sounds great :) It's a breaking change in |
|
If I'm not wrong, there's some code to be remove from |
jorgefilipecosta
force-pushed
the
update/way-presets-are-merged
branch
from
44690d5
to
447af09
Compare
Hi @youknowriad, It may or may not be a breaking change on useSetting. My initial plan was to on useSetting('color.palette') return the "merged" palette as we do today (if there are user colors return user colors if not return theme colors if not return default colors. Let me know what you prefer useSetting('color.palette') returns the "merged" palette as we do today or seSetting('color.palette') returns the object with all origins. |
This has my preference but I don't think we need a way to return the "merged" colors since in the UI these are going to be shown separately if I'm not wrong. I guess this means we'd probably need to do things like this in the color hooks: |
|
👋 Want to make sure the direction here is clear to me. Would this summarize what this PR is trying to achieve?
{
"version": 1,
"settings": {
"color": {
"palette": [
{ "slug": "color-slug-1", "value": "color-value-2", "name": "color name 1" },
{ "slug": "color-slug-2", "value": "color-value-2", "name": "color name 2" },
]
}
}
}
{
"color": {
"palette": {
"core": [
{ "slug": "color-slug-1", "value": "color-value-2", "name": "color name 1" },
{ "slug": "color-slug-2", "value": "color-value-2", "name": "color name 2" },
],
"theme": [
{ "slug": "color-slug-1", "value": "color-value-2", "name": "color name 1" },
{ "slug": "color-slug-2", "value": "color-value-2", "name": "color name 2" },
],
"user": [
{ "slug": "color-slug-1", "value": "color-value-2", "name": "color name 1" },
{ "slug": "color-slug-2", "value": "color-value-2", "name": "color name 2" },
]
}
}
}
|
| // doesn't contain spaces. | ||
| self::append_to_selector( $selector, '.has-' . preg_replace( '/\s+/', '-', $value['slug'] ) . '-' . $class['class_suffix'] ), | ||
| array( | ||
| $values_per_origin = _wp_array_get( $settings, $preset['path'], array() ); |
There was a problem hiding this comment.
If I understood properly this PR's direction, it seems that we'll always enqueue all the preset values (classes + custom properties) for every origin.
This makes me think of what happens when different origins have the same slug for the preset value. For example, if both core and a theme provides a color with yellow slug. Then, the user modifies the value of that yellow and we end up with:
body {
--wp--preset--color--yellow: <core_value>;
--wp--preset--color--yellow: <theme_value>;
--wp--preset--color--yellow: <user_value>;
}
.has-yellow-color { <core_value> !important; };
.has-yellow-color { <theme_value> !important; };
.has-yellow-color { <user_value> !important; };I'm wondering if a different implementation for compute_preset_classes and compute_preset_vars would make the code clearer and remove the duplicates. What if we:
- Reduced the
settings.color.paletteto a single object with slug and value. This step already takes care of the duplicates (the later origin to be processed wins). Something like:
{
<slug-1>: <value-1>,
<slug-2>: <value-2>,
...
}- Maped this object to the functions's output (either classes or custom properties).
There was a problem hiding this comment.
Actually. More thoughts for when two yellows are defined in different origins. Overwriting the previous origin works for now with the use case we have in mind: make those classes and custom properties available to patterns, independently from the theme in use.
However, when we want to show the colors in separate slots in the UI, how does that work if they have the same slug? Both when themes provide a color with the same slug than core's and when the user edits the value of a core or theme color. (Not that we need to implement it in this PR, but, at least, we should understand how feasible it is using this approach.)
There was a problem hiding this comment.
I applied your feedback! In the future, when implementing the UI, we may need to do something smart, and if the a color with the same slug existis in a higher priority origin, the UI stops showing the slug with lowest priority
There was a problem hiding this comment.
A braindump of the options I see:
- If theme colors use an existing core slug, the core color is overwritten. The current proposal. Gives us a way to update core colors, we'd need something separate if we wanted to remove them (perhaps a
coreColors: falseor a list of allowed colors). - If theme colors use an existing core slug, the theme color is removed. The way themes update/remove core colors is by updating a specific key in
theme.json, such ascoreColors: [ ... ].
Given that the reason we want core colors available is to have consistent CSS for use in patterns, removing them without alternative doesn't seem like something we need. So, it boils down to optimize for how we want updating to work. The option 2 is more intentional and straightforward (doesn't require we do anything smart for the UI).
If we went with option 2, we could do it in a follow up.
There was a problem hiding this comment.
I think what we have right now is enough we can overwrite the previous color slugs. There is now a way to remove them and that's our objective, in previous versions of WordPress there is also no way to remove core color classes one can just overwrite, we are doing the same behavior.
lib/global-styles.php
Outdated
| isset( $colors_by_origin['theme'] ) ? | ||
| $colors_by_origin['theme'] : | ||
| $colors_by_origin['core'] | ||
| ); | ||
| unset( $settings['__experimentalFeatures']['color']['palette'] ); |
There was a problem hiding this comment.
It seems we want this data in the client, to be accessed by useSetting( 'color.palette.origin' ), correct?
There was a problem hiding this comment.
Yes in the future we want it but for now, we can not add it. The way we are back-compatible with plugins changing the old color settings is by using unset( $settings['__experimentalFeatures']['color']['palette'] );. If we don't do this $settings['__experimentalFeatures']['color']['palette'] will take priority over the old settings and these plugins break. That was the reason we were already doing unset( $settings['__experimentalFeatures']['color']['palette'] ); and I don't think we can stop doing that here. I'm not sure how we are going to address this in the future.
There was a problem hiding this comment.
Hi @nosolosw I ended up removing these unsets I tested with the plugin Block Editor Colors and a theme without theme.json and the plugin still works as expected.
There was a problem hiding this comment.
According to the direction #32358 (comment):
The legacy settings at core/block-editor.settings.colors still hold only the colors in use by the UI controrls (either core or theme colors).
In addition to no longer unset the settings.__experimentalFeatures.colors we still need to retrofit the values to the old ones. Otherwise, what happens is that settings.colors holds incorrect data. Steps to reproduce with colors (same for gradients and font sizes):
- use WordPress 5.7, TT1-blocks, and activate this branch as the plugin
- go and see the values in the
core/block-editorstore undersettings.colors - the expected result was that it held the theme values but instead it had the default core colors
There was a problem hiding this comment.
We are now retrofitting the values to the old ones.
Exactly that's the direction this PR is going. |
jorgefilipecosta
force-pushed
the
update/way-presets-are-merged
branch
from
447af09
to
67d8f3e
Compare
jorgefilipecosta
requested review from
ajitbohra,
cameronvoell and
guarani
as code owners
|
Hi @youknowriad I applied the logic we discussed. useSetting( color.palette' ) returns an object with all origins, useSetting( color.palette.theme' ) returns a specific origin. We have a util __experimentalGetHighestPriorityPreset that returns the higher priority origin to avoid repeating colorPalette.user ?? colorPalette.theme ?? colorPalette.core everywhere. |
| @@ -3,6 +3,16 @@ | |||
| */ | |||
| import { SETTINGS_DEFAULTS } from '../store/defaults'; | |||
|
|
|||
| export function __experimentalGetHighestPriorityPreset( | |||
There was a problem hiding this comment.
Can this be absorbed by the useSetting hook? I imagine this could work this way:
useSetting( 'color.palette' ): returns the consolidated / merged palette.useSetting( 'color.palette.core' ): returns the core colors.useSetting( 'color.palette.theme' ): returns the theme colors.useSetting( 'color.palette.user' ): returs the user colors.
Similarly for the other presets.
There was a problem hiding this comment.
I suggested this option in #32358 (comment).
@youknowriad seemed to prefer to avoid this logic inside useSetting #32358 (comment).
There was a problem hiding this comment.
I'm hesitant :) it feels like we shouldn't allow color.palette entirely
There was a problem hiding this comment.
Is your concern it won't serve any purpose with the upcoming UI changes? Mine is similar: introducing a temporary __experimentalGetHighestProiorityPreset that will be deprecated, with the additional burden it poses in the consumers (they need to know the inner details of how this works).
Thinking out loud: the settings.colors should still have the proper values for the UI #32358 (comment) so I wonder if a way to avoid introducing this temporary utility would be to access settings.colors directly (same for gradients and font sizes)?
There was a problem hiding this comment.
Is your concern it won't serve any purpose with the upcoming UI changes?
Yes, it's not entirely clear how the global color.palette is going to be used. __experimentalGetHighestProiorityPreset is experimental so it's fine but remains the question about color.palette
There was a problem hiding this comment.
I think you should trust your instinct here, I personally don't have the right answer :)
There was a problem hiding this comment.
Thinking out loud: the settings.colors should still have the proper values for the UI #32358 (comment) so I wonder if a way to avoid introducing this temporary utility would be to access settings.colors directly (same for gradients and font sizes)?
using settings.colors is not an option because now settings depend on the block and the old settings structure does not allow that.
| // either remove it and append the incoming, | ||
| // or update it with the incoming. | ||
| $to_append = array(); | ||
| $to_append[] = array( 'color', 'duotone' ); |
There was a problem hiding this comment.
We're missing duotone from the list.
Naming: with the new behavior for merge to_replace / to_append no longer make sense, we could look for better names to both these. Perhaps $properties and $propertis_per_origin?
There was a problem hiding this comment.
I don't think duotone should be part of this list. For the duotone case, we don't have classes or CSS variables it is a server-side dynamic mechanism.
There was a problem hiding this comment.
Yeah, my point is that we're missing it: it's not in any list at the moment (either $properties or $to_append).
| @@ -140,14 +140,7 @@ function_exists( 'gutenberg_is_edit_site_page' ) && | |||
|
|
|||
| // Copied from get_block_editor_settings() at wordpress-develop/block-editor.php. | |||
| $settings['__experimentalFeatures'] = $consolidated->get_settings(); | |||
| if ( isset( $settings['__experimentalFeatures']['color']['palette'] ) ) { | |||
There was a problem hiding this comment.
Leaving a comment here to point to this thread.
| @@ -70,6 +70,11 @@ export const PRESET_METADATA = [ | |||
| }, | |||
| ]; | |||
|
|
|||
| const presetPaths = {}; | |||
There was a problem hiding this comment.
I don't see this in use anywhere?
|
Ok, I've reviewed this all. We need a couple of fixes: the legacy settings ( My main concern, though, is how consumers access the data ( |
jorgefilipecosta
force-pushed
the
update/way-presets-are-merged
branch
from
f094717
to
4bfddb3
Compare
jorgefilipecosta
force-pushed
the
update/way-presets-are-merged
branch
4 times, most recently
from
b310de7
to
d9bdb5b
Compare
jorgefilipecosta
force-pushed
the
update/way-presets-are-merged
branch
from
d9bdb5b
to
f5c1f87
Compare
|
The feedback on this PR was addressed. useSetting('color.palette') now returns the merged colors, useSetting('color.palette.theme') now returns theme colors. |
There was a problem hiding this comment.
I've tested a number of scenarios in the post & site editors using TwentyTwentyOne and TT1-blocks:
- a theme that does not define any color palette => core colors are shown
- a theme with an empty array as color palette => no colors are shown
- a theme with a color palette => the theme colors are shown
- add user colors via the site editor => the modifications are shown everywhere
I've also verified that the classes and custom properties for core are present in addition to theme CSS.
Everything is fine with 5.7. In WordPress 5.8 the list of colors shown include both core and theme, but this is expected because this reverts a change we landed in 5.8 and we still need to port this PR to core. Once we do, it'll work as 5.7.
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
Created https://core.trac.wordpress.org/ticket/53378 and WordPress/wordpress-develop#1363 to track this. |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. git-svn-id: https://develop.svn.wordpress.org/trunk@51149 602fd350-edb4-49c9-b593-d223f7449a82
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. git-svn-id: https://develop.svn.wordpress.org/trunk@51149 602fd350-edb4-49c9-b593-d223f7449a82
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. Built from https://develop.svn.wordpress.org/trunk@51149 git-svn-id: http://core.svn.wordpress.org/trunk@50758 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. Built from https://develop.svn.wordpress.org/trunk@51149 git-svn-id: https://core.svn.wordpress.org/trunk@50758 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…ta 2. This includes: **Various** - Fix multi selection for nested blocks WordPress/gutenberg#32536 - Consistently show the drop indicator while dragging blocks WordPress/gutenberg#31896 - Fix horizontal drop indicator WordPress/gutenberg#32589 - Fix Safari flickering issue WordPress/gutenberg#32581 - Silence useSelect zombie bug errors WordPress/gutenberg#32088 **Template Editor** - Clarify the template creation modal WordPress/gutenberg#32427 - Only add skip links for block templates WordPress/gutenberg#32451 **Widgets Editor** - Add block breadcrumb WordPress/gutenberg#32498 WordPress/gutenberg#32528 WordPress/gutenberg#32569 - Saved deleted and restored widgets. WordPress/gutenberg#32534 - Fix unsaved changes detection WordPress/gutenberg#32573 - Fix button spacing in the header WordPress/gutenberg#32585 - Avoid extra undo levels WordPress/gutenberg#32572 - Move Legacy Widget block to the `@wordpress/widgets` package WordPress/gutenberg#32501 - Fix Social Links color inheritance WordPress/gutenberg#32625 - Use Button appender WordPress/gutenberg#32580 **Global Styles (theme.json)** - Separate the presets per origin in the block editor settings WordPress/gutenberg#32358 WordPress/gutenberg#32622 - Use CSS Custom Properties for the preset styles WordPress/gutenberg#32627 **Performance** - Remove is-typing classname to improve typing performance WordPress/gutenberg#32567 Props nosolosw, jorgefilipecosta, aristath, ntsekouras, peterwilsoncc, mcsf. See #53397. Built from https://develop.svn.wordpress.org/trunk@51149 git-svn-id: http://core.svn.wordpress.org/trunk@50758 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This PR refactors our approach to Enqueue core and theme colors, gradients, font sizes, and font families all the time to use a data structure where each origin is saved separately.
This PR essentially reverts what was done in #31669 and tries a different approach.
This approach uses a data structure where the merged color.palette instead of containing an array of colors contains an object with { core, theme, user } each property being an array of colors of that origin. The complexity we have is because theme.json, core-theme.json, and the user json file don't contain origin, there color.palette is an array of colors, which makes the shape resulting from the merge different from the shapes that originated it.
This change opens the path for a future where multiple palettes are shown e.g. useSetting( 'color.pallete' ) may return user, or theme, or core while useSetting( 'color.pallete.theme' ) returns a specific origin.
In terms of simplification, this PR is not probably a win, but it also did not make things more complex than what we had, and it opens the door for the future where multiple origins are shown (now we have the data structure for that).
I did not test everything yet, and this PR still needs some polishing, but the general idea is already implemented, so this PR is open for feedback.
cc: @nosolosw, @youknowriad