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

Invalid unit type when insert number in number control #291

Closed
likern opened this issue Oct 19, 2021 · 8 comments
Closed

Invalid unit type when insert number in number control #291

likern opened this issue Oct 19, 2021 · 8 comments
Labels
6.5 bug Something isn't working good first issue Good for newcomers

Comments

@likern
Copy link

likern commented Oct 19, 2021

Describe the bug
When I use control type number for property

    monthsBefore: {
      control: {
        type: 'number'
      }
    }

preview rerenders immediately while I'm typing new value.
That leads to invalid empty string being passed to as number value (when I delete old number and try to insert new number).

Expected behavior
Don't rerender immediately, only when Submit button on keyboard was pressed.

Screenshots
Screenshot_2021-10-19-20-08-48-260_com reactnativeui
Screenshot_2021-10-19-20-08-02-372_com reactnativeui
Screenshot_2021-10-19-20-07-56-758_com reactnativeui

System:

Environment Info:
  System:
    OS: Linux 5.14 Fedora 34 (Workstation Edition) 34 (Workstation Edition)
    CPU: (8) x64 Intel(R) Core(TM) i7-4770K CPU @ 3.50GHz
  Binaries:
    Node: 16.10.0 - ~/.nvm/versions/node/v16.10.0/bin/node
    Yarn: 1.19.0 - ~/.nvm/versions/node/v16.10.0/bin/yarn
    npm: 7.24.0 - ~/.nvm/versions/node/v16.10.0/bin/npm
  Browsers:
    Chrome: 94.0.4606.81
    Firefox: 93.0
  npmPackages:
    @storybook/react-native: ^6.0.1-alpha.6 => 6.0.1-alpha.6
@dannyhw
Copy link
Member

dannyhw commented Oct 20, 2021

@likern thanks for reporting this. I can see why this is an issue.

I suppose that on blur or similar could be used to defer the update until the user is finished.

If you want to create a pr with your suggested solution that would be great. If not that's no problem, I'll make sure to include a fix before 6.0.

@likern
Copy link
Author

likern commented Oct 20, 2021

@dannyhw Unfortunately, I'm very busy with my duties. I can only report issues I've found while integrating StoryBook. Not able to contribute to this project.

@dannyhw
Copy link
Member

dannyhw commented Oct 20, 2021

@likern no problem, thanks for reporting issues, that's already a big help 🙏

@dannyhw dannyhw added good first issue Good for newcomers bug Something isn't working 6.5 labels Nov 2, 2021
@raychanks
Copy link
Contributor

@dannyhw Would like to contribute to this one.

I found that if onBlur is used, when the user finishes changing the number input and press the preview or navigator tab at the bottom or pressing the component preview section, the onBlur handler will not be called. (I am testing on an iOS simulator)

Also, I found that the type of argument that the onChange handler receives is of type string after the user changes the number input. Would it be better to ensure the argument type to always be number instead of sometimes string and sometimes number? I think it would mitigate this issue and some other issues like <Text>{1 + val}</Text>

@dannyhw
Copy link
Member

dannyhw commented Jan 10, 2022

@raychanks hey thats great, all contributions welcome.

I've tried before to resolve this however I didn't find a simple solution.

On the one hand I understand that its unfortunate when the app can crash from the wrong input however I think that it should be possible for the user to test these kinds of cases with storybook and im not sure we should sanitise input. You could make the argument that the users components should be resilient to undefined values but I can understand it from both sides.

Doing a onblur or on submit seems like a good way around the current functionality however as it currently works when you press the preview the addons panel is immediately unmounted and any on blur is not executed which leaves you with and awkward user experience. Using onsubmit and blur together could provide something workable but getting the blur to trigger might require some wrangling on the ondevice code.

Feel free to hack away at it, I would love to see what solution you come up with and I'm open to different approaches.

@raychanks
Copy link
Contributor

@dannyhw hacked a version to defer the change by handling it on blur and submit, please help reviewing it when you have time.

On the other hand, I would like to continue the discussion about the data type issue. I am still quite new to this project and not so familiar with how the users normally use storybook. I agree that storybook should allow users to test different input types to make sure the components they built are resilient enough to handle different cases. However, I think these kind of tests should be performed by the user explicitly, for example changing the control type in a story from number to text to explicitly test for the behavior of passing in a string.

At the time being, the NumberType input will implicitly changes the data type from number to string after modification, and the data type will keep on being string in subsequent changes, if the user actually wants the input type to be number at all times, we seem to not have a way to cater for it.

@dannyhw
Copy link
Member

dannyhw commented Jan 12, 2022

@raychanks thats an interesting point actually I guess you're right. I'm open to enforcing the number values.

Thanks a lot for opening the PR for this, I'll take a look soon :).

@raychanks
Copy link
Contributor

@dannyhw Oh if that's the case, I think the changes can be simplified and do not need to rely on the submit and blur handlers. I will update the code later tonight and mark the pull request as WIP for now. Thanks for your effort too :)

raychanks added a commit to raychanks/react-native that referenced this issue Jan 13, 2022
raychanks added a commit to raychanks/react-native that referenced this issue Jan 13, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 7, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 7, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 7, 2022
dannyhw pushed a commit that referenced this issue Feb 7, 2022
dannyhw pushed a commit that referenced this issue Feb 7, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 7, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 7, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 7, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 8, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 8, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 8, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 8, 2022
dannyhw pushed a commit to raychanks/react-native that referenced this issue Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.5 bug Something isn't working good first issue Good for newcomers
Projects
Archived in project
Development

No branches or pull requests

3 participants