Conversation
|
|
andrewserong
approved these changes
Nov 9, 2022
Contributor
andrewserong
left a comment
There was a problem hiding this comment.
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?
Contributor
|
Woops, just realised the missing CHANGELOG — I'll take care of it in a follow-up! Sorry |
Contributor
Author
|
Thank you for taking care of this! 🙇 |
Member
|
Nice one! This annoyed me a lot 😀 |
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
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 #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
onChangeinput, so that it's converted to0whenever 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