[base-ui][useNumberInput] Integrate useNumberInput with useControllableReducer #40206
Conversation
mj12albert
added
package: base-ui
Netlify deploy preview@material-ui/unstyled: parsed: +1.28% , gzip: +0.93% Bundle size reportDetails of bundle changes (Toolpad) |
mj12albert
force-pushed
the
base/number-input-reducer
branch
8 times, most recently
from
05c7b4e
to
c6bc84c
Compare
mj12albert
force-pushed
the
base/number-input-reducer
branch
from
e5c49cd
to
7ece809
Compare
github-actions
bot
added
the
PR: out-of-date
mj12albert
force-pushed
the
base/number-input-reducer
branch
from
7ece809
to
04b2d0d
Compare
github-actions
bot
removed
the
PR: out-of-date
There was a problem hiding this comment.
It seems to work well. One minor thing I noticed when I played with the demos (it's also present on master) - when there was no value in the input and I pressed the increment button, I subconsciously expected 1 instead of 0 to appear. This is how the native control works (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number)
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "props": { | |||
| "defaultValue": { "type": { "name": "any" } }, | |||
| "defaultValue": { "type": { "name": "number" } }, | |||
Good catch ~ I've updated it to match the native behavior in 57b8259 |
| React.useEffect(() => { | ||
| if (isControlled && isNumber(value)) { | ||
| dispatch({ | ||
| type: NumberInputActionTypes.resetInputValue, | ||
| }); | ||
| } | ||
| }, [value, dispatch, isControlled]); |
There was a problem hiding this comment.
@sai6855 I've adapted your fix (and the test ❤️ ) in #40348 here, as this PR will replace the manual setStates
I think it shouldn't be necessary to check for potential strings because it's not allowed for the value, and TS/PropTypes can take care of that. Also when it's uncontrolled, the inputValue should not ever get out of sync with the value like in https://github.com/mui/material-ui/issues/40340
For this, I've added an additional resetInputValue action CC @michaldudak
| it('sets value to max when the input has no value and ArrowDown is pressed', async () => { | ||
| const handleChange = spy(); | ||
| describe('when the input has no value and ArrowDown is pressed', () => { | ||
| it('sets value to max when max is provided', async () => { |
There was a problem hiding this comment.
This doesn't match the native behavior either - the min or -1 value should be used here (see the first demo in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number)
| it('sets value to min when the input has no value and ArrowUp is pressed', async () => { | ||
| const handleChange = spy(); | ||
| describe('when the input has no value and ArrowUp is pressed', () => { | ||
| it('sets value to min if min is provided', async () => { |
There was a problem hiding this comment.
This isn't quite how the native control works - it kinda assumes that no value = 0 (so if min = 0, pressing the ArrowUp sets the control to 1.
There was a problem hiding this comment.
@michaldudak This was intentional ~ I think it's a worthwhile improvement from the native behavior 😬
If a range is defined with min and/or max in a cleared NumberInput, it feels like a better UX to start incrementing from min on arrow up, and start decrementing from the max on arrow down
If arrow down starts on the min value, a 2nd arrow down will do nothing, and I feel its more natural to tap the same arrow 2-3 times – e.g. increment from min to min+3, or decrement from max to max-3 – rather than "down-up-up-up"
And luckily since the native number input isn't used in the wild that much, it shouldn't be something a typical web user would need to "unlearn", what do you think?
PS: Did a quick check as well, react-aria(react-spectrum) also implemented this behavior – sandbox - but mantine and Chakra do not
There was a problem hiding this comment.
BTW I would love to get @colmtuite's opinion on this as well ~
There was a problem hiding this comment.
Yes I agree @mj12albert. When min or max is set, it should increment/decrement from those values imo.
There was a problem hiding this comment.
All right, then. Let's get it merged!
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
Continues #38723
Closes #38514
Major changes:
When the input element is blurred,
onChangeis only called if the value has changed. Previously it was always called on blur even if the value remained the same.The
valuecan benullwhen the component has no value. The (custom)onChangewill be called withnullinstead ofundefinedas the second argument when there is no value (e.g. when the<input>has been cleared). For the values ofvalueanddefaultValue,undefinedis only used for determining whether the component is controlled or uncontrolled.The
valueis no longer changed by theinputChangeaction (typing in the<input>), it will only change when the<input>is blurred,Actions contain the underlying event (it was left out in [base-ui][NumberInput] Implement
numberInputReducer#38723), they are irrelevant to the reducer but need to be passed through to the state change callbackI have followed (at least) the PR section of the contributing guide.