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

[NumberInput] no way to get value from NumberInput if not numeric #7039

Closed
2 tasks done
davidicus opened this issue Oct 13, 2020 · 6 comments · Fixed by #11230
Closed
2 tasks done

[NumberInput] no way to get value from NumberInput if not numeric #7039

davidicus opened this issue Oct 13, 2020 · 6 comments · Fixed by #11230

Comments

@davidicus
Copy link
Contributor

What package(s) are you using?

  • carbon-components
  • carbon-components-react

Detailed description

Describe in detail the issue you're having.

While chrome prevents NumberInput from receiving a text values, both Firefox and Safari will allow text to be written to input. Without the feature flag flipped there is no way to receive an onChange event or handle this with an error message. This component should log all changes in the onChange handler.

Is this issue related to a specific component?

NumberInput

What did you expect to happen? What happened instead? What would you like to
see changed?

I expect onChange handler to be called anytime the value changes on the NumberInput. Currently onChange is not called when entering in text and if you add a ref the value that is sent back is empty.

What browser are you working in?

Chrome, firefox, and safari

What version of the Carbon Design System are you using?

latest

What offering/product do you work on? Any pressing ship or release dates we
should be aware of?

AI PAL

Steps to reproduce the issue

  1. Open up storybook in Firefox
  2. Add text to input field.
  3. There is no error message and no onChange is fired

Please create a reduced test case in CodeSandbox

https://codesandbox.io/s/elegant-lewin-hxhjc?file=/src/index.js

@JordanWSmith15
Copy link

@emyarod This issue is effecting the AI Applications MAS 8.3 release in January. Is there an expected resolution?

@emyarod
Copy link
Member

emyarod commented Oct 22, 2020

I'm not sure what the resolution to this might be since number inputs don't seem to accept non-numeric values by default

@davidicus
Copy link
Contributor Author

davidicus commented Oct 22, 2020

After further inspection I see that this is actually a bug in firefox and safari not in Carbon. There seems to be dispute in the application of the spec as far as rendering text in the UI when the value of the input is reported as an empty string.

I played around a bit and I believe we can add some validation to help catch these values. @JordanWSmith15 your devs can implement as is with the current implementation. @emyarod if this seems like something that should be built into the NumberInput I would be happy to contribute this fix.

Basically we just need to pass a validation function to onKeyUp on the NumberInput

const [isValid, setIsValid] = useState(true);
  const numberRef = useRef();

  const handleKeyPress = (e) => {
    console.log({ value: numberRef.current.value });
    if (
      !/^[1-9]\d*(\.\d+)?$/.test(e.key) &&
      numberRef.current.value.length === 0
    ) {
      setIsValid(false);
    } else {
      setIsValid(true);
    }
  };
  return (
    <NumberInput
      ref={numberRef}
      allowEmpty={true}
      invalid={!isValid}
      onKeyUp={handleKeyPress}
    />
  );

See a codesandbox here

@davidicus
Copy link
Contributor Author

Is this something that Carbon wants to handle for their consumers? If so I would be happy to contribute a fix.

@tw15egan
Copy link
Member

@joshblack any problems with adding this in? Seems like it would solve an issue for @davidicus and he's willing to contribute a fix 😃

@aledavila
Copy link
Contributor

@davidicus hey coming back to this. Seeing how its still an issue we would be happy to accept this contribution

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

Successfully merging a pull request may close this issue.

5 participants