v5.1.x change breaks Boolean knob

[metasean]

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 || ''

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

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]

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 || '',

[leoyli]

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

[stale]

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!

[leoyli]

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]

@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]

@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]

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.