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

fix(Slider): check validity of number input after onChange #9132

Merged
merged 10 commits into from
Sep 13, 2021

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Jul 7, 2021

Closes #8842

This PR updates Slider so that form validation happens after any changes are made. Previously, number input wouldn't allow you to backspace if the number would be invalid, even if you were changing it to something valid.

If, for example, you have min 30, and your current value is 35, if you want to change it to 34, the box wont allow you to delete the ‘4’ as the value would be ‘3’, even though you are still editing the number.

Changelog

New

  • isValid state
  • onBlur event handler for form validation

Changed

  • onChange handler no longer has form validation
  • fixed padding for slider invalid input (had extra padding from regular number input invalid styles which have an invalid icon, but slider does not have icon).

Testing / Reviewing

  • For testing, go to the Slider story, and use the text input to type an invalid number (test both less than the min and more than the max). After clicking away, the invalid state should appear. Then update the text input to a valid number and click away. The invalid state should disappear now.

@jnm2377 jnm2377 requested a review from a team as a code owner July 7, 2021 06:48
@jnm2377 jnm2377 requested review from joshblack and emyarod July 7, 2021 06:48
@netlify
Copy link

netlify bot commented Jul 7, 2021

❌ Deploy Preview for carbon-react-next failed.

🔨 Explore the source changes: 23fc148

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/613fa859d6117300085675a5

@netlify
Copy link

netlify bot commented Jul 7, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 23fc148

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/613fa8596b2cbb0008d2ac63

😎 Browse the preview: https://deploy-preview-9132--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Jul 7, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 23fc148

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/613fa859246a3e000701cc76

😎 Browse the preview: https://deploy-preview-9132--carbon-components-react.netlify.app

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like even if an invalid="false" prop is provided the component will be rendered as invalid no matter what, so I think we need to handle props and state sync

Screen.Recording.2021-07-07.at.10.03.32.mov

I believe this would fix the interactions with the controlled slider example as well

Screen.Recording.2021-07-07.at.10.06.43.mov

@jnm2377
Copy link
Contributor Author

jnm2377 commented Jul 7, 2021

it seems like even if an invalid="false" prop is provided the component will be rendered as invalid no matter what, so I think we need to handle props and state sync

@emyarod Do you have any suggestions on how to do that? I guess, I'm thinking maybe kinda how we merge refs in ComboBox? Is that what you meant?

ref={mergeRefs(textInput, rootProps.ref)}

@emyarod
Copy link
Member

emyarod commented Jul 7, 2021

most of our class components will perform a props/state sync in getDerivedStateFromProps along with any relevant change handlers. maybe the NumberInput would be a good reference to look at due to the similarities to this

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

Successfully merging this pull request may close these issues.

Slider Component input box bug
4 participants