RangeControl: clamp initialPosition between min and max values #42571
Conversation
ciampo
requested review from
mirka,
stokesman,
aaronrobertshaw,
andrewserong and
chad1008
ciampo
added
[Type] Bug
| // Value should be clamped on initial load | ||
| expect( numberInput?.value ).toBe( '100' ); | ||
| expect( rangeInput?.value ).toBe( '100' ); | ||
|
|
||
| fireChangeEvent( numberInput, '50' ); |
There was a problem hiding this comment.
This unit test is intentionally written using fireEvent and checking directly the input's value, so that it aligns to the style used for the rest of RangeControl's unit tests.
It would be great to refactor these tests to use user-event, but I currently don't have capacity
There was a problem hiding this comment.
It works well now and until/unless user-event adds any sort of feature to support range inputs, I'd say this suite should be last on the list for such a refactor.
| @@ -50,7 +50,10 @@ export function useControlledRangeValue( | |||
| const { min, max, value: valueProp, initial } = settings; | |||
| const [ state, setInternalState ] = useControlledState( | |||
| floatClamp( valueProp, min, max ), | |||
| { initial, fallback: null } | |||
| { | |||
| initial: floatClamp( initial ?? null, min, max ), | |||
There was a problem hiding this comment.
The ?? null is to fix a type mismatch, since:
initialcan benumber | undefinedfloatClampacceptsnumber | nullas its first argument
There was a problem hiding this comment.
This is working nicely for me, too, and confirmed that it resolves the bug reported in the other review in Storybook, and I didn't encounter any issues while smoke testing RangeControl components in the editor.
Totally optional, but is it worth updating the wording in the readme for the initialPosition prop? Currently it says:
If no value exists this prop contains the slider starting position.
Perhaps:
If no value exists this prop contains the slider starting position. The initialPosition will be clamped between the provided min and max prop values.
LGTM! 🎉
There was a problem hiding this comment.
I might be a bit late to this party but thanks for following up on this one @ciampo 👍
The fix and the doc updates LGTM!
✅ Could replicate the original problem
✅ Unit tests pass, and no typing errors
✅ This PR fixes the issue
✅ Component functions well in storybook and editors.
What?
Clamp the value of
initialPositionbetweenminandmax, so that theRangeControl's value (and in particular, the value displayed in theNumberControl) is withinmin/maxbounds.Why?
This fixes a bug noticed in #40535 (review)
How?
Wrapped
initialPositionin afloatClampcall inside theuseControlledRangeValuehook.Testing Instructions