material-ui
material-ui copied to clipboard
[base] Create useNumberInput and NumberInputUnstyled
This PR will add useNumberInput
and NumberInputUnstyled
to base.
Summary
-
useNumberInput
implements a number field component that currently supports positive and negative integers, and buttons that can increment and decrement the value. It's mostly adapted fromuseInput
. -
NumberInputUnstyled
usesuseNumberInput
internally and is the component counterpart. - In addition to the native
onChange
of the inner<input>
, aonValueChange
callback is provided that only fires when the value changes to a valid (clamped) value
Preview: https://deploy-preview-36119--material-ui.netlify.app/base/react-number-input/
Netlify deploy preview
@material-ui/unstyled: parsed: +4.79% , gzip: +4.71%
Bundle size report
Details of bundle changes (Toolpad) Details of bundle changes
Generated by :no_entry_sign: dangerJS against 420053dc0fd4a8ee922377cb42d1892aa0626f2c
When should onChange be called?
@mui/core After using my own implementation, I'm starting to think it's not the best 🥲. I wrote up alternatives in the PR description above, would appreciate your thoughts the options I've came up with or other suggestions!
In terms of the open question regarding the
onChange
fire, I would recommend keeping it similar with how the native input number is working. People can still useonBlur
if they want behavior similar to the other option.
I’ve revised the handlers like this:
-
onChange
,onBlur
,onFocus
are native - howeverevent.target.value
may be dirty -
onValueChange
is a handler that is called whenever the “true” (clamped) value changes, it could be by interacting with the<input>
or the stepper buttons. The callback is gives that latest value as an argument.
I think onValueChange
should cover most use cases, as it should guarantee the latest correct value from the component, and many other libraries have an api like this.
However when considering integration with FormControlUnstyled
I think it's better to expose the event too (so the signature becomes like useSelect)
Wrote the docs, only tests left!
I notice a strange behavior when I press keyup, the cursor move to the left of the input.
https://user-images.githubusercontent.com/18292247/231931706-aa2211fd-aa35-483d-b87d-e905c4ea49ca.mov
I notice a strange behavior when I press keyup, the cursor move to the left of the input.
We probably just need to prevent default/stop propagating, as otherwise the up and down arrow in the input move the cursor to the start/end of the text.
On strange behavior that I noticed is that if I click on one of the up or down button and while it is focused I start using the up and down arrow keys, the pages starts to scroll. I would recommend capturing these keys on the buttons too, and continue to update the input value. This is how the native input type="number" behaves.
Base UI number input:
Native number input
Looking awesome, just adding a couple of design tweaks!
@danilo-leal Thanks for the polish 🙏
I would recommend capturing these keys on the buttons too, and continue to update the input value.
@mnajdova I've tweaked this according to the ARIA spinbutton pattern:
The text field is usually the only focusable component because the increase and decrease functions are keyboard accessible via arrow keys
Now when clicking on the buttons, the input is focused automatically, subsequent key presses of supported keys (arrow up/down, page up/down etc) will continue to update the value.
https://github.com/mui/material-ui/assets/4997971/d17ae4a9-0c63-4cb5-bc4d-d2d12cce9979
PS this also fixes the jumping cursor position @siriwatknp
This is coming along very nicely 👍
For the docs (likely for a follow-up, so we can keep this PR smaller, focused on the foundations), I would love to see a few more demos, coming as a developer that is comparing different components, and wants to quickly pick one:
- min or max. I would like to see if the down button becomes greyed when we reach the min. It doesn't in our case.
- step. I would like to see if
step={0.1}
would work correctly. It doesn't in our case. - format. Say we display a number with
%
.
I don't have any remarks on the implementation.
I've noticed that the Import section on the component API tab is incorrect, as it doesn't take the Unstable
prefix into consideration.
The Import section on the hooks API tab is partially wrong (import { useNumberInput } from '@mui/base';
)
Things for the docs infra:
Imports are incorrect:
Two more things that I could find:
The arrows are off:
And the components imports are weird:
@michaldudak @mnajdova I've addressed the remaining comments!
In particular:
- To fix the import paths, I modified the docs pages components: https://github.com/mui/material-ui/commit/b887e89d38b64ffd281fbe0d58c488b529592fe5
- I noticed native CSS nesting doesn't work in Firefox yet (it's behind an experimental flag) and opened a separate issue: https://github.com/mui/material-ui/issues/38266
- After failing to align the arrows properly with CSS, I found it there may be an issue with the "small downward arrow" in Plex Sans... ended up using
system-ui
for arrows
The component import is still incorrect:
import Unstable_NumberInput as NumberInput from '@mui/base/Unstable_NumberInput';
should be
import NumberInput from '@mui/base/Unstable_NumberInput';
The component import is still incorrect:
👌 Fixed in https://github.com/mui/material-ui/pull/36119/commits/652c650aca6fd729ccd7e7968fd6bf36a06684d8 @michaldudak
@mj12albert Nice! A couple of opportunities I can notice in https://mui.com/base-ui/react-number-input/:
- [x] We had no GitHub issue for this new component, done
- [x] We were not linking this PR with the issue, done
-
[x] 1 Action vertical alignment bug: mui/material-ui#38521
-
[x] 2 Step rounding bug mui/base-ui#41
- [x] 3 Indentation issue on the docs page, should remove the tabs:
https://github.com/mui/material-ui/pull/38521
-
[x] 4 Special keyboard for mobile, it seems to use the regular keyboard layout now, see https://react-spectrum.adobe.com/blog/how-we-internationalized-our-numberfield.html#mobile for what I mean. mui/base-ui#40
-
[ ] 5 A missed opportunity for Twitter? https://twitter.com/devongovett/status/1389992231029800968 😁. @samuelsycamore I think this also can make the case for the benefit of https://www.notion.so/mui-org/Release-Batch-weekly-patches-341d87562b75473baa595ea30d496217.
-
[x] 6 I would love to see more live demos, e.g. step size, https://chakra-ui.com/docs/components/number-input/usage#setting-the-step-size, which I think depends a bit on https://www.notion.so/mui-org/Weekly-meeting-2023-08-14-af21836ef9af4f10b18875c61576148a?pvs=4#f21e5a831575454898f78676addd16ca, to not duplicate too much code. mui/material-ui#39274
-
[x] 7 Are we missing the use of
useControlled()
? There is no warning when changing control mode. mui/material-ui#38514
@mj12albert Thanks for the follow-ups on my comment. Opening individual issues for each point is the way to go 👌