material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[base] Create useNumberInput and NumberInputUnstyled

Open mj12albert opened this issue 2 years ago • 8 comments

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 from useInput.
  • NumberInputUnstyled uses useNumberInput internally and is the component counterpart.
  • In addition to the native onChange of the inner <input>, a onValueChange 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/

mj12albert avatar Feb 09 '23 11:02 mj12albert

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

mui-bot avatar Feb 09 '23 11:02 mui-bot

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!

mj12albert avatar Feb 24 '23 11:02 mj12albert

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 use onBlur if they want behavior similar to the other option.

I’ve revised the handlers like this:

  • onChange, onBlur, onFocus are native - however event.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)

mj12albert avatar Mar 03 '23 07:03 mj12albert

Wrote the docs, only tests left!

mj12albert avatar Apr 13 '23 10:04 mj12albert

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

siriwatknp avatar Apr 14 '23 03:04 siriwatknp

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.

mnajdova avatar Apr 14 '23 09:04 mnajdova

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:

number-input-bug

Native number input

number-input-native

mnajdova avatar Apr 14 '23 09:04 mnajdova

Looking awesome, just adding a couple of design tweaks!

@danilo-leal Thanks for the polish 🙏

mj12albert avatar Apr 19 '23 09:04 mj12albert

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

mj12albert avatar May 09 '23 11:05 mj12albert

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 %.

oliviertassinari avatar May 19 '23 17:05 oliviertassinari

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. image

The Import section on the hooks API tab is partially wrong (import { useNumberInput } from '@mui/base';)

michaldudak avatar Jun 26 '23 19:06 michaldudak

Things for the docs infra:

image

Imports are incorrect:

image

mnajdova avatar Jul 21 '23 13:07 mnajdova

Two more things that I could find:

The arrows are off:

image

And the components imports are weird:

image

michaldudak avatar Jul 31 '23 17:07 michaldudak

@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

Screenshot 2023-08-01 at 9 38 16 PM

mj12albert avatar Aug 01 '23 15:08 mj12albert

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';

michaldudak avatar Aug 02 '23 09:08 michaldudak

The component import is still incorrect:

👌 Fixed in https://github.com/mui/material-ui/pull/36119/commits/652c650aca6fd729ccd7e7968fd6bf36a06684d8 @michaldudak

mj12albert avatar Aug 02 '23 09:08 mj12albert

@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
Screenshot 2023-08-15 at 02 38 51
  • [x] 1 Action vertical alignment bug: mui/material-ui#38521 Screenshot 2023-08-15 at 02 20 33

  • [x] 2 Step rounding bug mui/base-ui#41

Screenshot 2023-08-15 at 02 23 10
  • [x] 3 Indentation issue on the docs page, should remove the tabs:
Screenshot 2023-08-15 at 02 24 32

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

oliviertassinari avatar Aug 15 '23 00:08 oliviertassinari

@mj12albert Thanks for the follow-ups on my comment. Opening individual issues for each point is the way to go 👌

oliviertassinari avatar Oct 03 '23 00:10 oliviertassinari