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

Reopen of 9472: Addons / Knobs - When type is Number using the number knob, throws an error on delete and field is null #6

Open
kmurgic opened this issue Feb 5, 2021 · 9 comments

Comments

@kmurgic
Copy link

kmurgic commented Feb 5, 2021

Quoting from storybookjs/storybook#9472 (closed as stale). I am still experiencing this issue, so I am reopening.

You have a knob, the prop type is Number and the default is number('something', 0). If the user deletes the 0 (or all digits in the field), an error is thrown in the console because the value cannot be null

vendors~main.5317d89c4b17337a8714.bundle.js:103113 Warning: value prop on input should not be null. Consider using an empty string to clear the component or undefined for uncontrolled components.

To Reproduce
Steps to reproduce the behavior:

  1. Set a prop using a number knob in a story

    hello: {
    type: Number,
    default: number('hello', 0),
    },

    Use it in the template
    Run the Storybook
    Navigate to the story, delete the value in that knob field
    See error in console

Expected behavior
There is no error when the text in a number input is deleted. I believe the value of the input is being set to null when it should be set to an empty string instead. The value of the knob could be null, but the value of the input element should be an empty string.

Additonal Small Requested Change
In TypeScript the value of a number knob is typed as number, leaving no way to initialize the number input as empty. It would be nice to allow an option to initialize the value of the knob as null .

System

Environment Info:

  System:
    OS: Linux 5.4 Ubuntu 20.04.2 LTS (Focal Fossa)
    CPU: (12) x64 Intel(R) Core(TM) i7-9850H CPU @ 2.60GHz
  Binaries:
    Node: 14.15.3 - ~/.nvm/versions/node/v14.15.3/bin/node
    Yarn: 1.22.10 - ~/.nvm/versions/node/v14.15.3/bin/yarn
    npm: 6.14.9 - ~/.nvm/versions/node/v14.15.3/bin/npm
  Browsers:
    Chrome: 88.0.4324.96
    Firefox: 85.0
  npmGlobalPackages:
    @storybook/addon-knobs: 6.2.0-alpha.23

Additional context
It looks like this issue has been brought up a few times, and possibly fixed.

storybookjs/storybook#9472 (closed as stale)
storybookjs/storybook#8882 (closed as stale)
storybookjs/storybook#5912 (closed as fixed)
storybookjs/storybook#2475 (closed as fixed)

@shilman
Copy link
Member

shilman commented Feb 6, 2021

FYI, we’ve released addon-controls in Storybook 6.0. Controls are portable, auto-generated knobs that are intended to replace addon-knobs.

Please upgrade and try them out today!

@kmurgic
Copy link
Author

kmurgic commented Feb 8, 2021

I just reached for knobs since that what I was familiar with from previous Storybook work. Sounds like this is a low priority bug then. I will experiment with migrating to controls. Thank you!

@shilman
Copy link
Member

shilman commented Feb 16, 2021

@kmurgic yup, planning to deprecate knobs in 6.2. please give controls a try!

@laddi-netapp
Copy link

@shilman this also happens in controls, btw 😉

@shilman
Copy link
Member

shilman commented Feb 19, 2021

@laddi-netapp not too surprising since controls is based on the knobs code. the difference is that it's much more likely to get fixed in controls 😉

@kmurgic
Copy link
Author

kmurgic commented Feb 22, 2021

@shilman this also happens in controls, btw wink

Just confirmed that I am getting the same issue in controls, when I delete the content of a number input.

Warning: `value` prop on `input` should not be null. Consider using an empty string to clear the component or `undefined` for uncontrolled components.

@kmurgic
Copy link
Author

kmurgic commented Apr 2, 2021

I can open a PR for this. I figured out a way to change the storybook code to fix it, but I'm not sure if it's the best/cleanest possible way.

@kmurgic
Copy link
Author

kmurgic commented Apr 2, 2021

I haven't really contributed to open source before, so apologies in advance if my PR and/or ability to follow the contributing process is lacking.

@kmurgic
Copy link
Author

kmurgic commented Apr 2, 2021

storybookjs/storybook#14457 should fix the issue.

@shilman shilman transferred this issue from storybookjs/storybook May 10, 2021
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

No branches or pull requests

3 participants