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: enforce number data type for number input (#291) #315

Merged
merged 8 commits into from
Feb 19, 2022

Conversation

raychanks
Copy link
Contributor

@raychanks raychanks commented Jan 12, 2022

Issue: #291

What I did

Enforce number data type for number input

How to test

Please explain how to test your changes and consider the following questions

  • Use a number input control in a story
  • Change the value of the input to see it's still a number when rendered in the component

Some cases to consider:

  • Input with a comma (e.g. 12,3), expect to see the comma replaced with a dot
  • Change the input value and then press on the preview section, expect to see the changes reflected
  • Input invalid number format (e.g 12...3), expect to see the input border color changes to signify invalid input
  • Clear the input will give a zero value
  • Put a - sign only will give NaN, while normal negative numbers will give the value of the number

  • Does this need a new example in examples/native?

No

  • Does this need an update to the documentation?

No

If your answer is yes to any of these, please make sure to include it in your PR.

@raychanks raychanks requested a review from dannyhw as a code owner January 12, 2022 16:55
@raychanks raychanks changed the title fix: defer change update for number input (#291) WIP: fix: defer change update for number input (#291) Jan 13, 2022
@raychanks raychanks changed the title WIP: fix: defer change update for number input (#291) fix: enforce number data type for number input (#291) Jan 13, 2022
@dannyhw
Copy link
Member

dannyhw commented Jan 13, 2022

thanks for the contribution 🙇

I will review it soon when I have some time.

@dannyhw
Copy link
Member

dannyhw commented Jan 18, 2022

sorry for the delay, I've not had the time yet. I haven't forgotten about this though.

@dannyhw
Copy link
Member

dannyhw commented Jan 25, 2022

@raychanks hey again sorry for taking so long.

Seems to solve the issue with an empty input so for the related issue its successful.

The only other thing I noted is that with invalid values the value becomes NaN. Is that what we want? I'm ok with it just wondering what you think about it.

@raychanks
Copy link
Contributor Author

@dannyhw sorry for the late reply. I think that invalid values should be passed in as NaN as NaN is of type number, and users should be able to test this case. However, one thing I am unsure of is the case of having just a minus sign -.

I am thinking about the use case that the user wants to input a negative number. The normal flow would be clearing the input field first, then input the - sign, and then type in the number. At the moment of having only the - sign, the value would be NaN. Should we cater for this case? I think the advantage of handling the - sign only would be not having NaN transient value, while the disadvantage is that we would have more hidden logic. What do you think?

@dannyhw
Copy link
Member

dannyhw commented Feb 7, 2022

@raychanks I think when its - we can just set -1 for that special case, I'll push a change for that. I'm going to give this another quick run and then merge.

Sorry it took so long to get this merged, I've just started a new job and so I have had less time recently.

@dannyhw
Copy link
Member

dannyhw commented Feb 7, 2022

I was trying to rebase this and I totally screwed up the entire thing 😩.
edit:
I think I managed to solve it, for a second there I thought it was unrecoverable 😰.

@dannyhw
Copy link
Member

dannyhw commented Feb 8, 2022

@raychanks hey I was just checking this one more time and the problem is that the reset doesn't work since the state of 'numStr' is not consistent with the arg state.

Not really sure what the right move is with this now honestly. I'll think on it.

@raychanks
Copy link
Contributor Author

@dannyhw no worries, best of luck to your new role.

Not sure whether a -1 is appropriate, the user might find it more difficult to input other negative numbers like -2. As soon as the user tries to delete the 1 from -1, the input will immediately restore to -1.

Nice catch on the reset bug, I will try to figure out a solution too.

@dannyhw
Copy link
Member

dannyhw commented Feb 8, 2022

@raychanks thanks for the kind words :)

in this case the -1 was only set to the Args not to the local state so it didn't effect the typing of the number. Feel free to adjust it though and handle in another way 🙂

@raychanks
Copy link
Contributor Author

@dannyhw you're right about the -1 case.

I added a isPristine state to keep track of the reset status. It should be working on the zero value case, reset case and regular number input case now.

@dannyhw
Copy link
Member

dannyhw commented Feb 11, 2022

@raychanks interesting change, I'll give it a try later. Looks good though!

@dannyhw
Copy link
Member

dannyhw commented Feb 19, 2022

@raychanks this is working great! Great job 🎉 .

Really appreciate your help, and I'm sorry again for taking so long to get to it.

Copy link
Member

@dannyhw dannyhw left a comment

Choose a reason for hiding this comment

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

🚀 🚀

Great work!

@dannyhw dannyhw merged commit 379da8e into storybookjs:next-6.0 Feb 19, 2022
@dannyhw
Copy link
Member

dannyhw commented Feb 19, 2022

published this change on 6.0.1-beta.3

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.

2 participants