ColorPalette: Update label correctly when value is CSS variable#41461
Merged
ColorPalette: Update label correctly when value is CSS variable#41461
Conversation
mirka
added
[Type] Bug
|
Size Change: +23 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
approved these changes
May 31, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
LGTM 🚀
- ✅ Code changes look good
- ✅ New Storybook example works as expected
- ✅ Original issue in the block editor is fixed
- ✅ Thank you for going the extra mile and adding unit tests 👏
Contributor
|
Thanks for fixing the issue! theme.json used for the test{
"version": 2,
"settings": {
"layout": {
"contentSize": "650px",
"wideSize": "1000px"
},
"color": {
"palette": [
{
"slug": "orange",
"color": "orange",
"name": "Orange"
},
{
"slug": "wp-preset-color",
"color": "var(--wp--preset--color--pale-pink)",
"name": "Preset Color"
},
{
"slug": "wp-preset-gradient",
"color": "var(--wp--preset--gradient--vivid-cyan-blue-to-vivid-purple)",
"name": "Preset Gradient"
},
{
"slug": "rgb",
"color": "rgb(34, 12, 64)",
"name": "RGB"
},
{
"slug": "rgba",
"color": "rgba(34, 12, 64, 0.6)",
"name": "RGBA"
},
{
"slug": "hsl",
"color": "hsl(30, 100%, 50%)",
"name": "HSL"
},
{
"slug": "hsla",
"color": "hsla(30, 100%, 50%, 0.6)",
"name": "HSLA"
},
{
"slug": "long-label-color",
"color": "#ff0000",
"name": "Long Long Long Long Long Long Label"
}
]
}
}
}fixcolor-palette-var.mp4Personally, I was concerned about the following two points, but I think they are out of the scope of this PR.
|
Member
Author
@t-hamano Sounds good, thank you. As for the latter point, I do think the uppercase is more consistent, but no strong opinion. Feel free to propose it if you like, and one of the designers can take a look! |
youknowriad
pushed a commit
that referenced
this pull request
Jun 3, 2022
* ColorPalette: Update label correctly when value is CSS variable * Add unit tests * Add story * Add changelog
|
I'm still seeing this issue. Is the fix released? |
Member
Author
If you're using the Gutenberg plugin, yes. Otherwise, I believe not. It will be released in WordPress 6.1. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fixes #41157
What?
Fixes a bug where the color name label would not update correctly when switching between colors that were CSS variables.
Why?
The
colord( value ).toHex()normalization that we were doing before finding a color match does not work for CSS variables, returning the fallback result of#000000. Because all CSS variables got normalized down to#000000, we could not reliably find the current color in the list of colors.How?
If a color value looks like a CSS variable, don't try to normalize it to a hex value.
Testing Instructions
npm run storybook:devKnown issue with text label contrast switching: #41459