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

v5.1.x change breaks Boolean knob #6366

Closed
metasean opened this issue Apr 1, 2019 · 7 comments
Closed

v5.1.x change breaks Boolean knob #6366

metasean opened this issue Apr 1, 2019 · 7 comments
Assignees
Milestone

Comments

@metasean
Copy link

metasean commented Apr 1, 2019

Describe the bug
Changing value to value: value || '' in addons/knobs/src/components/PropForm.js (introduced with bug fix #6043 😞 ) causes boolean knobs to return an empty string when toggled from true, which itself causes the error:

Warning: Failed prop type: Invalid prop `knob.value` of type `string` supplied to `BooleanType`, expected `boolean`.

To Reproduce
Steps to reproduce the behavior:

  1. Go an existing project with a Boolean knob
  2. in node_modules/@storybook/addon-knobs/dist/components/PropForm.js around line 76 change value: value || '' to just value (broken to fixed) or vice versa.
  3. Restart the server (e.g. yarn storybook)
  4. With console log open, toggle a boolean knob

Expected behavior
Toggling a boolean from true should not result in an error about an empty string.

Screenshots

Bad === value: value || ''

PropForm-Bad

Good === value === value: value,

PropForm-Good

Code snippets

  _createClass(PropForm, [{
    key: "makeChangeHandler",
    value: function makeChangeHandler(name, type) {
      var onFieldChange = this.props.onFieldChange;
      return function (value) {
        var change = {
          name: name,
          type: type,
          // value,            // <-- GOOD in v5.0.6
          value: value || '',  // <-- BROKEN in v5.1.x
        };
        onFieldChange(change);
      };
    }
  }, {

i.e. the opposite of bugfix #6043

Additional context
This boolean bug was introduced in a fix for changing a number value to an empty value (#6043). Fixes should account for both scenarios.

@metasean
Copy link
Author

metasean commented Apr 1, 2019

Using the following instead addresses the known situations, however I haven't tested it against all knob types, and ideally there will be tests in any pull request for this fix.

possible fix: value: value === true ? false : value || '',

@shilman shilman added this to the 5.1.0 milestone Apr 2, 2019
@stale stale bot added the inactive label Apr 23, 2019
@storybookjs storybookjs deleted a comment from stale bot Apr 23, 2019
@stale stale bot removed the inactive label Apr 23, 2019
@storybookjs storybookjs deleted a comment from github-actions bot Apr 23, 2019
@leoyli leoyli self-assigned this Apr 23, 2019
@leoyli
Copy link
Contributor

leoyli commented Apr 23, 2019

I think I can fix this when in the upcoming weekend :)

@stale
Copy link

stale bot commented May 14, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label May 14, 2019
@shilman shilman removed the inactive label May 14, 2019
leoyli pushed a commit that referenced this issue May 20, 2019
Use ES6 argument default value assignment to handle `undefined` case,
which is never wanted; this prevent `false` cast to empty string (bug).
@leoyli
Copy link
Contributor

leoyli commented May 20, 2019

Sorry for this easy bug I committed to fix lately, I get many things pile up unfortunately 😂... I think this commit should logically fixing the bug since it allow false (and potentially 0) to be correctly passed in.

leoyli added a commit that referenced this issue May 20, 2019
…olean-knob

BUGFIX v5.1.x change breaks Boolean knob (#6366)
@leoyli
Copy link
Contributor

leoyli commented May 20, 2019

@metasean,

Hey, thank you for pointing me a good direction to fix the bug, although I did not take you suggested bugfix approach. I would also encourage you to submit PR and invite people to review it next time :).

The fact is value could be 0, null, undefined, '', false, NAN that triggers this bug, and I believe the intention was to ensure the value can be typed against undefined, the true intention need more investigation though.

The PR should be included in the next release, by then, feel free to file another issue if you have other concerns. 👍

@metasean
Copy link
Author

@leoyli - No sweat from my end. If my pile hadn't been so high (and remained so high) I would have submitted a PR instead of a just a bug report. So at this point, I'm just happy you've submitted a fix! Thank you! 🙇‍♂️

@shilman
Copy link
Member

shilman commented May 21, 2019

Huzzah!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-rc.0 containing PR #6830 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants