Skip to content

Color Picker: Prevent all number fields to become 0 when one of them is an empty string#45649

Merged
ciampo merged 2 commits intotrunkfrom
fix/color-picker-number-controls-empty
Nov 10, 2022
Merged

Color Picker: Prevent all number fields to become 0 when one of them is an empty string#45649
ciampo merged 2 commits intotrunkfrom
fix/color-picker-number-controls-empty

Conversation

@Copons
Copy link
Copy Markdown
Contributor

@Copons Copons commented Nov 9, 2022

Fixes #36703

What?

This PR fixes an incorrect behaviour where, when deleting a value from the Color Picker's detailed inputs (e.g. the R value in RGB), all other inputs (e.g. the G and B in RGB) would be reset to 0.

Why?

Deleting a number field is a common user action. Resetting unrelated fields is an unexpected side effect with no obvious workarounds.

How?

The proposed change normalizes the onChange input, so that it's converted to 0 whenever it's an empty string or undefined.

The change is larger than strictly needed because of type discrepancies detected by TypeScript.


cc @mirka @ciampo:
I would have loved to remove the // @ts-expect-error TODO: Resolve discrepancy in NumberControl, which I think is reasonably resolved here.
Though, removing it triggers a new error with the return type, which probably requires more familiarity with TS than I have.

EDIT: in e8a99e6 I've tightened up the type checks (thanks to @ciampo for pointing out that the error was on the param, not on the return as I thought!), which allowed me to remove the linter exception. Let me know if this is a reasonable approach!


Testing Instructions

  1. Open a Color Picker, e.g. select a block and modify its Text color.
  2. Toggle Show detailed inputs.
  3. Select RGB in the dropdown that appears. (Bug also exists when using HSL.)
  4. Type in something like R = 200, G = 200, B = 200.
  5. Double click on the R field.
  6. Press Delete, as if you were about to modify it to be 100.
  7. ⚠️ Ensure the G and B fields keep their values.

@Copons Copons added [Package] Components /packages/components [Feature] Component System WordPress component system [Feature] Colors Color management labels Nov 9, 2022
@Copons Copons requested review from ciampo, mirka and noisysocks November 9, 2022 18:37
@Copons Copons self-assigned this Nov 9, 2022
@Copons Copons requested a review from ajitbohra as a code owner November 9, 2022 18:37
@codesandbox
Copy link
Copy Markdown

codesandbox bot commented Nov 9, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

Copy link
Copy Markdown
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is testing nicely for me!

✅ Confirmed that before this PR, clearing the field resets the other color values
✅ With this PR applied, the issue is resolved

Do you mind also adding an entry to packages/components/CHANGELOG.md?

@ciampo ciampo merged commit 3f5416f into trunk Nov 10, 2022
@ciampo ciampo deleted the fix/color-picker-number-controls-empty branch November 10, 2022 00:36
@github-actions github-actions bot added this to the Gutenberg 14.6 milestone Nov 10, 2022
@ciampo
Copy link
Copy Markdown
Contributor

ciampo commented Nov 10, 2022

Woops, just realised the missing CHANGELOG — I'll take care of it in a follow-up! Sorry

@Copons
Copy link
Copy Markdown
Contributor Author

Copons commented Nov 11, 2022

Thank you for taking care of this! 🙇

@noisysocks
Copy link
Copy Markdown
Member

Nice one! This annoyed me a lot 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Colors Color management [Feature] Component System WordPress component system [Package] Components /packages/components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Color Picker: RGB values reset to 0 when you delete text in one of the RGB fields

5 participants