Change Number input type to text instead of number

[vlzuiev]

There are multiple problems when it comes to number input e.g. there is an empty input ref value when the inputted value is not a number. So, in the context, you will have null but on UI you'll still see value with overlaying label.

For more limitations, you can read up on the following article:
https://technology.blog.gov.uk/2020/02/24/why-the-gov-uk-design-system-team-changed-the-input-type-for-numbers/
In addition to this MUI explicitly highlights that type='number' can be problematic.
https://mui.com/material-ui/react-text-field/#type-quot-number-quot

I'd attach two videos showing the buggy behavior and behavior after adjustments.

Screen.Recording.2024-04-05.at.11.52.28.mov

Screen.Recording.2024-04-05.at.11.53.35.mov

PS. It's possible to keep the type as a number. I can add some clean-up logic onBlur to explicitly set an empty string value to the input. I feel it's a bit hacky.

[vlzuiev]

The problem is coming from mui library.

Here is some code example how to reproduce the same with only mui component.

export const MuiError = () => {
    const [value, setValue] = React.useState('');

    return (
        <TextField
            label="some label"
            value={value}
            onChange={e => setValue(e.currentTarget.value)}
            type="number"
        />
    );
};

[fzaninotto]

I'm -1 for that change. NumberInput has limitations, but the alternative you propose isn't better IMO. It removes the ability to enter decimals, the step feature and the stepper buttons. As such, it's a big breaking change.

Besides, MUI advises using their NumberInput, but it's in preview, and it's not using material design. So until there is a clear better path, I think we shouldn't set the NumberInput to type="text".

[vlzuiev]

@fzaninotto The other option would be to create input ref if it's not passed and reset it's value if event value is undefined.
This allows for decimal numbers.

onBlur={(e) => {
          if (!e.currentTarget.value && inputRef.current) {
            inputRef.current.value = '';
          }
  }}

this will solve the buggy behaviour

[vlzuiev]

@fzaninotto I've reverted changes to keep the input type as number and adjusted the code to reset the input ref value on blur when the event value is empty.
Please let me know what you think of this change.

[vlzuiev]

any thoughts on those changes ?

[slax57]

Sorry we took so long to review your PR. Somehow it got under the radar.

This PR is also missing unit tests to prove that it works, and avoid these issues in the future.
Do you think you can add some?

Thanks?

[slax57]

I'm afraid !event.currentTarget.value will return false when the value is 0, which will reset the value although 0 is a valid value for NumberInput.