-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ColorPalette: make component more resilient to color entries with same color value
and/or name
#43197
Comments
Tagging:
|
It could be tricky 🤔🤔🤔🤔🤔🤔 I assume we only want to transform data in the editor JS so that the UI controls behave (?) I'm just thinking aloud... we could add unique keys in global-styles/hooks, where we build the colors object, e..g, if (
shouldDisplayDefaultColors &&
defaultColors &&
defaultColors.length
) {
result.push( {
name: _x(
'Default',
'Indicates this palette comes from WordPress.'
),
// Add a unique id
colors: defaultColors.map( ( item, index ) => ( {
...item,
key: `${ item.color }-${ index }`,
} ) ),
} );
} But then how would we determine whether the color is "selected" based on the value, when the only value we store is the raw color? Maybe, at the same time we store the selected color value, store a separate stringified color object as the "selected" color I'm not sure this is a good idea, but if anything better occurs to me I'll repost. |
…ndicator (#44344) * use slug as color indicator key in gs palette In the global styles panel's palette component, the color's hex code is used as a `key` for the ColorIndicatorWrapper, but the hex is not necessarily a unique identifier, unlike the `slug` attribute, which could be expected to be unique, at least by usage; discussion around improving handling of duplicate color in the palette [here](#43197). Composing the color + index of the element in the array, using it as an index, keeps these error messages away: > Warning: Encountered two children with the same key, `.$#040DE1`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version. with a palette that looks lilke this: ```json [ { "color": "#040DE1", "name": "Primary", "slug": "primary" }, { "color": "#040DE1", "name": "Foreground", "slug": "foreground" }, ] ``` * compose color, index as color indicator key
I'm coming here again from #49593 Several observations: 1. Remove duplicate preset slugsHere's what I'm seeing in the editor in the global styles CSS: --wp--preset--color--custom-color-1: #d7eb00;
--wp--preset--color--custom-color-1: #e61280;
--wp--preset--color--custom-color-1: #13f10e; And, in the frontend: --wp--preset--color--custom-color-1: #13f10e; Based on the following data in the global styles custom post type saved in the database: `"custom":[{"color":"#d7eb00","name":"Color 1","slug":"custom-color-1"},{"color":"#e61280","name":"Color 1","slug":"custom-color-1"},{"color":"#13f10e","name":"Color 1","slug":"custom-color-1"}]}}}` Where there are duplicate preset slugs, the frontend deliberate removes them and will only use the last known duplicate. See WP_Theme_JSON_Gutenberg::get_settings_slugs. So I think the first step is for the editor to do what the frontend does - only use the last known one when generating the CSS. The editor, via getPresetsDeclarations is inconsistent in that it allows duplicates when outputting styles. Theoretically, there won't be any regressions since the frontend has never allowed duplicates, and, as #49593 demonstrates, the last duplicate will conquer regardless. The editor must, however, do something extra: ensure it uses the last known duplicate when rendering the color palette picker as well. It will be debatable where exactly to do this, and whether to do it for all origins and not just custom. My instinct is to create a utility hook that parses the output of 2. Already-saved preset duplicatesBecause some users may have saved duplicates in the palettes, we should still show them in the palette. However, once point 1 is done, the editor should show a warning on the color palette in global styles. Something along the lines of: "Yo, there are duplicates and some of them won't work, please rename if you have time. Good day to you!" 3. Disallow duplicate slugsThe final step, and probably the trickiest is to not allow saving identical slugs in the first place. Maybe the optimal user experience would be at the data entry stage. So, check for duplicates before merging with the current global styles object and either:
|
I've been playing around with a very hacky branch that looks at duplicate slugs in custom presets: #65597 It's only to illustrate and experiment with my notes above. Since the slug is derived from the name I guess it would be okay. I'm running on the assumption that we'd allow duplicate colors. I don't see any harm in having 10 x However, what causes the main issues are the duplicated slugs, since these are used as ids across the UI. |
+1
Don't we want to remove the duplicate preset slugs from the merged data across origins? I might be missing something though 🤔
I think either of those makes sense. We might also want a combination where if it is just the slug that is a duplicate create the unique slug but if the slug and color match an existing color warn and discard it. |
Do you mean in the settings itself? Theme_JSON doesn't care about duplicate slugs in the settings itself, only at the point of CSS output. So I'm thinking we should preserve the source data in the editor as well. If you mean dedupe for every origin (not just custom) for the purposes of building CSS and displaying options in the UI, then 💯 I'll test that in #65597 soon. I think that would match what Theme_JSON does: it performs the deduping at the CSS-generation stage by slug per origin. It favours the last known duplicate slug within a specific origin. So, given --wp--preset--color--base: #111111;
--wp--preset--color--custom-base: #00ff51; Here is how the controls behave in the UI (in trunk): 2024-09-25.10.25.34.mp4 |
This but I think I was a bit confused/short on caffeine. The rest of your explanation makes sense thanks 👍 |
The UX/UI is the trickiest bit. Here's one, low-effort option: Kapture.2024-09-26.at.11.52.10.mp4 |
Question from someone who doesn't really know how the back-end works: could there ever be a name conflict between two theme-defined colors, or even a "standard" variable and a "theme" variable? In other words: could there ever be a conflict that is not caused by the user adding a custom variable? Showing a warning to the user doesn't feel like a good solution to me — although I realise that I'm just pointing out an issue without providing a better solution. Without much knowledge of how the backend works, could we generate a unique ID for each color entry ? |
Good question. It depends what you mean by "conflict" though 😄 The backend explicitly overwrites duplicate slugs: The consequence is that only the last-known duplicate preset will be used to output preset CSS vars. This will occur for all origins — base, theme, custom — but not between origins, since they have different prefixes, e.g., So, the Theme_JSON class will try to avoid conflicts this way, and do its best to output CSS without taking down a site, but there are no warnings or PHP errors.
I agree it's not ideal. In #65597 I was limiting it to custom colors only, so the user has an opportunity to address it seeing as it's unlikely they can remove dupes in theme.json. The idea behind it was to communicate to the user that they have (or someone with the same access has) created custom colors with the same name/slug. They might be wondering why their custom styles aren't appearing or don't work in the UI. I'm not sure where else to do this.
This was step three in my 3-step plan 🤣 The slugs are generated using the name input value, so we can definitely test for that. But it won't fix the bug for users who have already created duplicates, hence the warning suggestion. |
WIP PR here: |
What problem does this address?
Initially flagged and discussed in #43096
While working on separate issues, I noticed that the
ColorPalette
component can receive a listcolors
where two or more color entries can have the samevalue
(e.g#ffffff
).When this happens, currently the
ColorPalette
component is not able to correctly highlight as "selected" the color that was actually picked by the user — instead, it highlights all color entries sharing that samevalue
as "selected":(image taken from this comment
Having different entries sharing the same color
value
(and potentially even the same colorname
) should be considered as a totally valid scenario:Text
with value#000000
, and another entry calledButtonText
with value#000000
Primary
and value#ff0000
. The user then also defines an entry with the same and same valueThis means that the
ColorPalette
component should not attempt to introduce any logic to alter the list of color entries in order to mitigate this issue, as that could result in an unexpected behavior to the user.What is your proposed solution?
While there isn't a commonly agreed solution yet, one way to look at this issue is to make sure that the data representing the color entries (passed to
ColorPalette
) is improved in a way that makes each color entry uniquely identifiable, even if it shares the samename
andvalue
as another entry (e.g. anid
prop).I can only imagine that this wouldn't be a trivial change, since it would involve potentially changing the schema of the data, and making sure that the
id
is generated correctly regardless of how the color is added to the list of available colors for a site (e.g. via the global styles UI, by editing theme.json...).Since I'm not very knowledgeable about this part of the Gutenberg app, I'll let other folks chime in and discuss potential solutions (I'll keep an eye in case the conversation involves the
ColorPalette
component)The text was updated successfully, but these errors were encountered: