Make user able to change all color palette origins #36674
Merged
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
jorgefilipecosta
added
Backport to WP 6.7 Beta/RC
jorgefilipecosta
requested review from
ajitbohra and
spacedmonkey
as code owners
|
Size Change: +325 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/user-edits-all-origins
branch
2 times, most recently
from
1d3f76e to
f8bd70c
Compare
jorgefilipecosta
force-pushed
the
update/user-edits-all-origins
branch
from
f8bd70c to
fb6d2fa
Compare
|
Hey, I've tested this and it's working nicely. Need to take a look at the code (will do it today before lunch). There's only one thing we should fix before landing: the user can introduce spaces (and other characters) in the custom color slug, so we should sanitize this (no spaces, valid characters), etc. Other things/follow-ups/up for discussion:
|
oandregal
reviewed
Nov 22, 2021
| @@ -1333,48 +1335,52 @@ public function merge( $incoming ) { | |||
| * @return array | |||
| */ | |||
| private static function remove_insecure_settings( $input ) { | |||
| $origins = array( 'default', 'theme', 'user' ); | |||
There was a problem hiding this comment.
Can we use the VALID_ORIGINS here?
oandregal
reviewed
Nov 22, 2021
| @@ -301,7 +301,9 @@ public function __construct( $theme_json = array(), $origin = 'theme' ) { | |||
| $path = array_merge( $node['path'], $preset_metadata['path'] ); | |||
| $preset = _wp_array_get( $this->theme_json, $path, null ); | |||
| if ( null !== $preset ) { | |||
| _wp_array_set( $this->theme_json, $path, array( $origin => $preset ) ); | |||
| if ( 'user' !== $origin || isset( $preset[0] ) ) { | |||
There was a problem hiding this comment.
I'd think we want to rename the user origin to custom, as we did with core to default to match the UI, class/css var generation, etc. Perhaps we can do this in a different PR that lands before this one?
There was a problem hiding this comment.
What's the use case isset( $preset[ 0 ] ) is checking for? My understanding is that we should always skip reorganizing the presets if the origin is the user, as we'll store them already keyed by origin, so this could be simplified to: if ( null != $preset && 'user' !== $origin ).
Additionally, I wonder if we need to keep back compatibility with existing user data in the database. We could add an extra step to normalize the user data by origin for that case (older data that is not keyed by origin).
There was a problem hiding this comment.
Oh, I see now: the isset( $preset[ 0 ] ) already does the back-compatibility trick :) Only old user data with no keys by origin would have the 0 key.
oandregal
reviewed
Nov 22, 2021
| isSmall | ||
| isPressed={ isAdding } | ||
| icon={ plus } | ||
| label={ __( 'Add custom color' ) } |
There was a problem hiding this comment.
It sounds like the ColorEdit component should be agnostic as to what's the source of the color. I'd think this label should be provided by the consumer or be called Add color.
oandregal
reviewed
Nov 22, 2021
| <ColorNameInput | ||
| value={ color.name } | ||
| onChange={ ( nextName ) => | ||
| onChange( { | ||
| ...color, | ||
| name: nextName, | ||
| slug: kebabCase( nextName ), | ||
| slug: slugPrefix + kebabCase( nextName ), |
There was a problem hiding this comment.
Ah, I see. We need to fix the kebabCase import to import { kebabCase } from 'lodash';.
oandregal
reviewed
Nov 22, 2021
| color, | ||
| onChange, | ||
| isEditing, | ||
| onStartEditing, | ||
| onRemove, | ||
| onStopEditing, | ||
| slugPrefix, |
There was a problem hiding this comment.
I'm not very keen on making the ColorEdit component aware of the prefix for custom colors. For components, do we usually expose "hooks" for consumers to tweak the normal behavior? Like, for example, we could have an onNameChange callback that consumers can use to tweak the name as they like (add a prefix, etc).
Not really familiar with how/whether this is a practice we want our components to have, so I'll go with whatever you think is best.
There was a problem hiding this comment.
I think the prefix may end up being used for other things besides custom colors, and it seems we will probably not need other name manipulation than the prefix. So I guess slugPrefix is enough. In the future, I'm not totally sure of the API and the place where this component will be. I guess when we remove experimental from this component or move the component to another package we should audit its API.
oandregal
reviewed
Nov 22, 2021
| paletteLabel={ __( 'Theme' ) } | ||
| /> | ||
| ) } | ||
| { !! defaultColors && !! defaultColors.length && ( |
There was a problem hiding this comment.
We also need to consider themes that opt-out from the default palette via settings.color.defaultPalette.
|
Hi @oandregal,
I will try to address that but I think it should be in another PR that is an issue we already had and not related to the changes we are doing here.
No, one can not directly edit color by pressing the color circle. We are keeping the same behavior as before. In the mockups, I don't see colors being edited by pressing the circle so I don't think that is expected.
Custom is part of the slug and we use "--" to separate paths in the preset, so in some parts of the engine may be "custom--red" would not be interpreted as a slug. Also, the classes would have strange names like "custom--red-background-color". I would prefer to keep the prefix "custom-", as using "custom--" breaks the promise that "--" is a separator mark in the preset path, (the slug itself would have that mark). |
oandregal
approved these changes
Nov 22, 2021
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Nov 23, 2021
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.
This PR makes the user able to edit default, theme, and custom color palettes. For the default and theme the user can only change color values.
For the custom palette, the user can change everything.
The user can reset each palette individually.
It also prefixes also custom colors slugs with "custom-".