Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Number input type to text instead of number #9757

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vlzuiev
Copy link

@vlzuiev vlzuiev commented Apr 5, 2024

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.

There are multiple problems when it comes to number input e.g. there is empty input ref value when 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 following article.
https://technology.blog.gov.uk/2020/02/24/why-the-gov-uk-design-system-team-changed-the-input-type-for-numbers/
@vlzuiev
Copy link
Author

vlzuiev commented Apr 5, 2024

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
Copy link
Member

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
Copy link
Author

vlzuiev commented Apr 5, 2024

@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

when inserting incorrect number value the input state is set to null.
However, input ref still has incorrect value and causes input label to overlay value which is still shown.
@vlzuiev
Copy link
Author

vlzuiev commented Apr 6, 2024

@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
Copy link
Author

vlzuiev commented Apr 10, 2024

any thoughts on those changes ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants