Skip to content
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 utils: do not normalize undefined color values #64969

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Sep 2, 2024

What?

Fixes a regression introduced by #64224

normalizeColorValue() was treating empty style values as non-simple values and therefore returning defaultView?.getComputedStyle

Adds tests

Why?

Empty values were being filled by an element's computed background color, which could be the site background color.

This is confusing and means that, by default, it's impossible to select a new color without adjusting the alpha value.

How?

Detecting false values and returning the value immediately.

Testing Instructions

  1. Open the editor and insert a group block
  2. Open the color picker for background or any other color style
  3. Ensure that you can see your custom color preview immediately in the canvas
  4. Check the test description of ColorPalette: partial support of color-mix() CSS colors #64224 and ensure that works

Before

The default alpha value is 0, which means, by default, any custom color cannot be seen in the canvas.

Kapture.2024-09-02.at.14.46.23.mp4

After

The alpha is 100 so, by default, my custom color changes are visible on the canvas.

Kapture.2024-09-02.at.14.44.36.mp4

Semi-related

…le values as non-simple values and therefore returning defaultView?.getComputedStyle

adding tests
@ramonjd ramonjd added [Type] Regression Related to a regression in the latest release [Package] Components /packages/components labels Sep 2, 2024
@ramonjd ramonjd requested review from mirka and ciampo September 2, 2024 05:22
@ramonjd ramonjd self-assigned this Sep 2, 2024
@ramonjd ramonjd requested a review from ajitbohra as a code owner September 2, 2024 05:22
@ramonjd ramonjd changed the title Color utils: do not normalize undefined color values ColorPalette utils: do not normalize undefined color values Sep 2, 2024
Copy link

github-actions bot commented Sep 2, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@@ -92,9 +92,13 @@ export const normalizeColorValue = (
value: string | undefined,
element: HTMLElement | null
) => {
if ( ! value || ! element ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a related conversation about how to handle null values here: #64224 (comment)

Not sure if I've grokked the outcome of that, so let me know if there's a better way to do this.

@ramonjd
Copy link
Member Author

ramonjd commented Sep 2, 2024

Check the test description of #64224 and ensure that works

Seems to work for me

2024-09-02.15.29.58.mp4

Copy link

github-actions bot commented Sep 2, 2024

Flaky tests detected in d0d336e.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10661442034
📝 Reported issues:

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, the code works as intended. Thank you 🙏

@@ -39,5 +39,24 @@ describe( 'ColorPalette: Utils', () => {
'#ff0000'
);
} );
test( 'should return the value as is if the color value undefined', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I entirely get the meaning of this test name

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 bad copy pasta. I'll fix - thanks for noticing

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix @ramonjd 🙌

@ramonjd
Copy link
Member Author

ramonjd commented Sep 2, 2024

I appreciate the quick reviews, folks 🙇🏻

@ramonjd ramonjd merged commit 341b5e8 into trunk Sep 2, 2024
62 checks passed
@ramonjd ramonjd deleted the fix/color-picker-normalize-color-alpha branch September 2, 2024 09:00
@github-actions github-actions bot added this to the Gutenberg 19.2 milestone Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants