Color Palette: Respect defaultPalette setting (fixes Cover block placeholder state) #58869
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Regression
andrewserong
requested review from
youknowriad,
noisysocks,
matiasbenedetto,
richtabor,
ramonjd,
tellthemachines and
aaronrobertshaw
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -40 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Whoops! I missed that unit tests will need updating, too, I see the Cover block ones are failing now. I'll take a look 👀 |
Updated in f51fc73 — it turns out the tests depended on the bug, as they first created the block by selecting a colour from the default palette, which shouldn't have been rendered. That's fixed up now, and I believe this PR should be ready for review. |
|
Flaky tests detected in f51fc73. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7839380140
|
In WP 6.4 running TT4 without Gutenberg, toggling Or is that intentional? 😅 |
|
I'm having difficulty working out what the intended behaviour of gutenberg/schemas/json/theme.json Line 148 in 5939c41 Based on #39966, it seems that the intention is that it only controls whether the default palette appears in this popover:
(Both screenshots taken in an environment running WP 6.4 without the plugin.) So I think what I noticed in #58869 (comment) is a bug/regression and that the placeholder should always only show theme colours. |
In the context of this PR, yes, that was my intention — it ensures the behaviour is consistent with the Background or Color panels where if the default palette is enabled, the default colors are available to choose from: 
It's a good question, but I think the answer depends on what we think of the discussion from #55219 — prior to that PR (in 6.4), the |
|
Update: after discussing some of the ideas here with @noisysocks, I've pushed an update to the Cover block so that it'll only show the default color palette if there are no theme colors. So, in a site that has custom and theme colors, and with 
Some of the considerations thus far:
@richtabor / @WordPress/gutenberg-design does this PR roughly match expected behaviour now? |
There was a problem hiding this comment.
Thanks for working on this @andrewserong 👍
It's testing as advertised for me.
Given @noisysocks discussions with you it might be prudent to wait on his approval though.
Also, given we have some tests for the Cover block it might be good to lean into that and add one here around the available colors in the placeholder. What do you think?
| @@ -318,7 +318,19 @@ function CoverEdit( { | |||
| const blockProps = useBlockProps( { ref } ); | |||
|
|
|||
| // Check for fontSize support before we pass a fontSize attribute to the innerBlocks. | |||
There was a problem hiding this comment.
Should we update this comment to reflect recent changes? There's a bit more going on now than just checking fontSize support.
There was a problem hiding this comment.
Oh, yes, of course, I'll update this tomorrow 👍
Great idea. I'll let this PR simmer overnight, and then I'll add in another one for the new behaviour here 👍 |
youknowriad
added
the
Backported to WP Core
andrewserong
added
Backport to WP 6.7 Beta/RC
|
Update: from the discussion over in #55219 (comment), it sounds like folks might be considering going with reverting #55219, in which case we wouldn't need this fix in the shorter-term. Just in case, I've updated this PR with a few more tests, so that if the decision is not to revert that PR, we can still go with this fix. Happy to either go with this, or close it out as needed! |
|
Let's see what comes of #58951 first. |
|
Closing in favour of #58951 since it has a few approving reviews. Assuming that one lands, I might open a follow-up PR that is fairly similar to this one, but an enhancement for 6.6 instead of a bug fix for 6.5. That is, to allow combined custom color + theme color buttons in the Cover block's placeholder, as with #58951 the custom color palette will override the theme color palette instead of the two being merged. |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
deleted the
fix/default-palette-setting-in-color-palette-component
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes: #58753
Update the
ColorPalettecomponent used by the Cover block's placeholder state so that it is aware of thecolor.defaultPalettesetting, and therefore can skip outputting the default palette when required.Why?
This was raised in: #58753 and from a git bisect, it seems that the PR that originally caused this to regress was: #55219
If my understanding is correct, #55219 resulted in combining the values for all origins into
color.palettesince it's included in the__EXPERIMENTAL_PATHS_WITH_MERGEobject here. So, after that PR, and prior to this one,'color.palette'will automatically include all the default palettes in it.So that means one solution is to:
How?
custom,theme, anddefaultcolors separately, and combine them, skipping thedefaultcolors ifcolor.defaultPaletteis falsy.color.defaultPaletteis not enabled.colorsvalue, that will only show the default palette colors if there are no theme colors, and if thecolor.defaultPalettevalue is truthy.I'm not sure if this is the best way to fix it, but it seems to do the job so far!
Testing Instructions
trunkif you add a Cover block to a post, page, or template, you'll see the default colors in addition to theme or custom colors in the placeholderdefaultPalette), you should only see theme + custom colors in the placeholdersettings.color.defaultPaletteintheme.json— switching it totrueshould re-enable the display of the default colors again — they will be visible when highlighting text, and in the using color pickers. In the Cover block placeholder, however, we only show the default palette if there are no theme colors available.Screenshots or screencast
color.defaulPaletteis set totrue)This should also fix an issue with the colors displayed when highlighting text, too: