RangeControl: Fix number input change interaction #22084
Conversation
This update fixes the number input field that renders within RangeControl to allow for (keyboard) changes to happen before it gets applied (or submitted). This temporarily bypasses the validation step, which accommodates cases where the current value (e.g. 1) may be lower than the min value (e.g. 10) while typing. The unit tests have RangeControl have been refactored to use @testing-library/react. This enables more accurate rendering as the (number) input value is applied only on blur or on ENTER keypress.
ItsJonQ
added
[Type] Bug
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
|
Size Change: +405 B (0%) Total Size: 825 kB
ℹ️ View Unchanged
|
| @@ -61,6 +61,7 @@ const BaseRangeControl = forwardRef( | |||
| onFocus = noop, | |||
| onMouseMove = noop, | |||
| onMouseLeave = noop, | |||
| resetFallbackValue, | |||
There was a problem hiding this comment.
What's the prop needed for? Could we include documentation?
There was a problem hiding this comment.
What's the prop needed for? Could we include documentation?
Still wondering about this one.
There was a problem hiding this comment.
Ah! Thanks for following up. It's the value to be used if the "Reset" button is clicked.
This was a prop that existed previously, but I guess didn't have docs. I'll add some.
| return; | ||
| } | ||
|
|
||
| handleOnSubmit( event ); |
There was a problem hiding this comment.
If I understand "submit" in this context to mean "propagate a value which is valid", I'd worry about the specific word "submit", since it typically carries a very different meaning when considering an input of a form (i.e. the form submission). At least, that was my first impression seeing this.
A word like "commit" or "persist" might be less prone to confusion?
There was a problem hiding this comment.
Thank you @aduth ! That makes sense. I think commit works! I'll make the update here (as well as my other PR that uses a similar flow)
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
|
@aduth (As always) Thank you for your thorough review!! I merged the suggestions you made, and added tests + comments for I made a minor update based on a suggestion by @mcsf from another one of my PRs
|
|
Hai! Following up on this one. Do you folks think this one is ready to land? Thank you! |
There was a problem hiding this comment.
This is definitely an improvement, and appears to resolve #21549 in my testing.
One thing I can't recall is if we'd previously supported temporarily, invalid non-numeric input (i.e. typing letters within these number fields). This doesn't appear to work at all, though it may very well be expected since it's a number input (possibly the browser preventing it).
@aduth Thank you so much for reviewing! I just checked a branch with the older version. It appears the |
|
Yay! Looks like Travis started. Will merge once Travis is happy. Thank you for the reviews! |
This update fixes the number input field that renders within RangeControl to allow for (keyboard) changes to happen before it gets applied (or submitted). This temporarily bypasses the validation step, which accommodates cases where the current value (e.g. 1) may be lower than the min value (e.g. 10) while typing.
The unit tests have RangeControl have been refactored to use @testing-library/react. This enables more accurate rendering as the (number) input value is applied only on blur or on Enter keypress.
These changes are important to fix certain RangeControl integrations like the one within the Latest Posts block. The GIF below demos the fix as the user can now type in values like
10. Previously, this did not work - Each key stroke would have been invalidated (keydown1, keydown0) in the process of typing10.Blurring or pressing Enter (new) applies the typed change. If valid, the change will go through. If not, the value will reset.
How has this been tested?
@testing-library/reactfor more accurate DOM testingChecklist:
Resolves: #21549