Global Styles: use color hex + index as key in the Palette component color indicator #44344
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
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 is
expected to be unique. Using slug as key prevents errors such as this:
> 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"
},
]
```
vcanales
added
[Type] Bug
|
Size Change: +1.77 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
Thank you for working on a fix for this warning!
I don't think that the slug can be unique, unfortunately — for example, if you add two custom color with the same name (e.g
Some time ago I had opened an issue related to this: #43197 I also had to create a similar fix, and I decided to use the |
I realize that the code itself does not restrict the slug's uniqueness at this time, but it seems like from a usage point of view, it makes sense to expect it to be unique.
This is what I was trying to avoid as well, but it does look like we don't have a choice right now! I've updated the PR to use your suggestion. |
ciampo
approved these changes
Sep 26, 2022
There was a problem hiding this comment.
Yup, I 100% agree with you — I also thought (as it's reasonable to assume) that slug is unique, but in reality the code doesn't force it, unfortunately (feel free to follow/join the conversation in #43197).
The fix, even if not ideal, does the job for now — thank you 🙏
vcanales
changed the title
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?
Use the color's
slugattribute from theme.json color palette configuration askeyforColorWrapperIndicatorcomponent.Why?
In the global styles panel's palette component, the color's hex code is used as a
keyfor the ColorIndicatorWrapper, but the hex is not necessarily a unique identifier, unlike theslugattribute, which is expected to be unique. Using slug as key prevents errors such as this:with a palette that looks lilke this:
[ { "color": "#040DE1", "name": "Primary", "slug": "primary" }, { "color": "#040DE1", "name": "Foreground", "slug": "foreground" }, ]Regardless of whether or not it's practical to have the same hex under two different color configurations, slug is the value expected to be unique, not the color itself.
Testing Instructions
Make sure that using the same color on two different palette color slots does't throw an error in the console.