Color UI component: reorder palettes and update names (core by defaults, user by custom) #36622
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
oandregal
added
the
Global Styles
|
Size Change: +112 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
oandregal
changed the title
oandregal
force-pushed
the
update/core-by-defaults
branch
from
d0c7933
to
936c2c8
Compare
oandregal
commented
Nov 18, 2021
|
|
||
| colorGradientSettings.colors = useMemo( () => { | ||
| const result = []; | ||
| if ( shouldDisplayCoreColors && coreColors && coreColors.length ) { |
There was a problem hiding this comment.
Per the mockups, the order in which the palettes should be shown is: defaults, theme, user.
oandregal
commented
Nov 18, 2021
| colorGradientSettings.gradients = useMemo( () => { | ||
| const result = []; | ||
| if ( |
There was a problem hiding this comment.
Per the mockups, the order in which the palettes should be shown is: defaults, theme, user.
oandregal
commented
Nov 18, 2021
| return useMemo( () => { | ||
| const result = []; | ||
| if ( coreColors && coreColors.length ) { |
There was a problem hiding this comment.
Per the mockups, the order of teh palettes is: theme, defaults, custom.
oandregal
commented
Nov 18, 2021
| return useMemo( () => { | ||
| const result = []; | ||
| if ( coreGradients && coreGradients.length ) { |
There was a problem hiding this comment.
Per the mockups, the order of teh palettes is: theme, defaults, custom.
oandregal
requested review from
ajitbohra,
ajlende,
ellatrix,
mkaz and
spacedmonkey
as code owners
oandregal
added
the
Backport to WP Beta/RC
oandregal
changed the title
Mamaduka
approved these changes
Nov 18, 2021
There was a problem hiding this comment.
Thanks, @oandregal.
The changes look good to me, and the new order matches the mockups
Comment on lines
-92
to
-93
| 'corePalette' => null, | ||
| 'coreGradients' => null, |
There was a problem hiding this comment.
I assume these settings were only recently introduced, which is why we don't need migrations.
carolinan
reviewed
Nov 19, 2021
There was a problem hiding this comment.
I have tested this manually by viewing the palettes in the global styles sidebar, site editor, and in the block editor.
This works well, I am very glad the use of "Core" was replaced.
I then removed the two default palettes using the new names in theme.json.
The default palettes are removed from the site editor and block editor, but not from the global styles, is that correct?
--
I have one related question, that should not be a blocker for this PR. The border color option only displays the custom color palette, is that correct?
|
@carolinan Thanks for the thorough testing, it's really helpful. Apparently, we missed using those flags in the site editor, I'll get that fixed. Need to investigate the border-color issue. |
10 tasks
|
#36639 fixes the flags in the GS sidebar. |
noisysocks
removed
the
Backport to WP Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Nov 22, 2021
…ts, user by custom) (#36622)
This was referenced Nov 29, 2021
Closed
57 tasks
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.
Based on #36496
Part of #36543 and #36556
In working with the new color UI component to show three different palettes we used these mockups #34574 which use the following names:
This PR updates:
corePaletteandcoreGradientsflags intheme.jsonto bedefaultPaletteanddefaultGradientsinstead.Follow-ups:
default,theme, anduserinstead ofcore,theme, anduser. I'd like to address this as a follow-up to control the PR size. This matters because they're part of the API we expose inuseSetting( 'color.palette.core' )so it should be coherent with the UI for simplicity.