Block Editor: Integrate slug-based color selection in color panel#78048
Block Editor: Integrate slug-based color selection in color panel#78048USERSATOSHI wants to merge 8 commits into
Conversation
135da60 to
7134bc2
Compare
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @DesignerThan95, @metasaman, @timelsass. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
With #78004 merged, I will review this PR once it gets rebased to include the |
|
cc @timelsass in case you want to confirm the fix, since you opened the original issue |
There was a problem hiding this comment.
Pull request overview
Integrates slug-based color selection into the Global Styles ColorPanel so preset colors with identical color values can be selected and persisted unambiguously (building on the ColorPalette slug-selection work).
Changes:
- Thread preset color slugs through Global Styles color panel controls and setters (encode by slug when available).
- Update
ColorGradientControlto passselectedSlugtoColorPaletteand propagate the selected slug throughonColorChange. - Add a Block Editor changelog entry documenting the duplicate-palette bug fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/block-editor/src/components/global-styles/color-panel.js | Extracts/passes color slugs through Global Styles color tabs and encodes preset colors by slug. |
| packages/block-editor/src/components/colors-gradients/control.js | Forwards selectedSlug to ColorPalette and propagates slugs in onColorChange. |
| packages/block-editor/CHANGELOG.md | Notes the Global Styles duplicate-palette slug-selection fix under Unreleased bug fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const colorPaletteOnChange = canChooseAGradient | ||
| ? ( newColor, _index, newSlug ) => { | ||
| onColorChange( newColor, newSlug ); | ||
| onGradientChange(); | ||
| } | ||
| : ( newColor, _index, newSlug ) => onColorChange( newColor, newSlug ); | ||
|
|
||
| const tabPanels = { | ||
| [ TAB_IDS.color ]: ( | ||
| <ColorPalette | ||
| value={ colorValue } | ||
| onChange={ | ||
| canChooseAGradient | ||
| ? ( newColor ) => { | ||
| onColorChange( newColor ); | ||
| onGradientChange(); | ||
| } | ||
| : onColorChange | ||
| } | ||
| selectedSlug={ colorSlug } | ||
| onChange={ colorPaletteOnChange } | ||
| { ...{ colors, disableCustomColors } } |
ciampo
left a comment
There was a problem hiding this comment.
Mostly looking good! Left a few smaller comments.
Will do a final round of review + smoke tests once those are addressed 🙏
| onColorChange={ | ||
| isGradient | ||
| ? undefined | ||
| : ( newColor, newSlug ) => setValue( newColor, newSlug ) | ||
| } |
There was a problem hiding this comment.
Could this be simplified to onColorChange={ isGradient ? undefined : setValue } to avoid re-creating a new function every re-render?
| const extractColorSlug = ( rawValue ) => { | ||
| if ( typeof rawValue !== 'string' ) { | ||
| return undefined; | ||
| } | ||
| if ( rawValue.startsWith( 'var:preset|color|' ) ) { | ||
| return rawValue.slice( 'var:preset|color|'.length ); | ||
| } | ||
| const themeFormatMatch = rawValue.match( | ||
| /^var\(--wp--preset--color--([^)]+)\)$/ | ||
| ); | ||
| return themeFormatMatch?.[ 1 ]; | ||
| }; |
There was a problem hiding this comment.
Could we move this function outside of the component render, and hoist it alongside DEFAULT_CONTROLS and popoverProps ? It would avoid re-creating it on every re-render.
Potentially even better, we could move it to a shared util. I can see some partial overlap in packages/block-editor/src/hooks/color.js. Very optional for this PR, but we could de-deuplicate the logic and update hooks/color.js to also accept the theme format (I think the logic from this PR is more correct)
| // Compare raw encoded references, not decoded color values: two palette | ||
| // entries can share the same hex value but carry different slugs | ||
| // (e.g. var:preset|color|dark-text vs var:preset|color|dark-background). | ||
| // Comparing decoded values would incorrectly conflate them, forcing the | ||
| // link preset to follow the text preset even though the user chose | ||
| // independent palette slots. | ||
| if ( | ||
| inheritedValue?.color?.text === | ||
| inheritedValue?.elements?.link?.color?.text | ||
| ) { |
There was a problem hiding this comment.
Just highlight a small semantic change between the new check and the old textColor === linkColor that was flagged during a round of AI-assisted review.
Previously, text and link would sync when their resolved colors matched; now they sync only when their raw stored references are identical strings. Edge case: if text was stored as var:preset|color|primary and link as the resolved #0000ff, the old behavior would sync them; the new behavior won't. In practice this is unlikely, and the new behaviour is more correct for the duplicate slug problem, but we may want to call it out in the PR description (and potentially CHANGELOG?)
There was a problem hiding this comment.
Maybe we could add some tests to check the new behaviour? Something like a small smoke test asserting that setTextColor('#000', 'dark-text') writes var:preset|color|dark-text (not a value lookup result), and maybe that the text↔link sync no longer triggers when raw refs differ even if decoded values match?

What?
Closes #9357
Follow up PR for #78004
Integrate slug-based color selection from the ColorPalette component into the Global Styles color panel. This is a follow-up PR to the components package fix that enables proper handling of palette entries with identical color values.
The original PR was splitted into 2 PRs based on the review provided.
Why?
The ColorPalette component now supports slug-based selection (see PR #78004) to correctly distinguish palette entries that share the same color value. This PR completes the integration by connecting that improvement to the WordPress editor's global styles system, enabling:
How?
extractColorSlug()helper to parse slug from CSS variable format (var:preset|color|slug)encodeColorValue()to accept optionalslugparameter and return the CSS variable directly when providedinheritedSlugthrough ColorPanelTab → ColorGradientControl → ColorPaletteTesting Instructions
#000000with different semantic names)theme.jsonto confirm the correct slug is saved (not a raw color value)Dependencies
This PR builds on #78004 (ColorPalette component changes). The components package establishes the slug-based selection mechanism; this PR integrates it end-to-end into the block editor's global styles workflow.