Fix the output of the boolean knob #3612
Conversation
Keraito
added
bug
addon: knobs
patch:yes
Keraito
requested review from
alexandrebodin,
alterx,
Hypnosphi,
leonrodenburg and
theinterned
as code owners
addons/knobs/src/registerKnobs.js
Outdated
|
|
||
| // Update the related knob and it's value. | ||
| const knobOptions = knobStore.get(name); | ||
|
|
||
| knobOptions.value = value; | ||
| if (type === 'boolean') { | ||
| knobOptions.value = !knobOptions.value; |
There was a problem hiding this comment.
what if the default value is true and initial toggled to false but since value is undefined, this will set knobOptions.value as true which is the wrong value.
I would suggest a undefined check.
There was a problem hiding this comment.
initial toggled to
false
What do you mean exactly? That the default value is true and the user toggles it to false? That the default is true and the user toggled it to false, and that toggle persisted after refresh? I'm kinda confused so could you elaborate more?
There was a problem hiding this comment.
@wuweiweiwu in my change, I'm using the knopOptions.value stored in the current knob to calculate the new knobOptions.value. So i'm not using the value destructured from the incoming change object. That value is always undefined, so it's impossible to defer the correct boolean from that value. The knob itself initializes its knobOptions.value with the setted default value, so my change is basically proposing to flip that value every time, instead of relying on the incoming event value. Does this solve your issue? Sorry for the confusion.
There was a problem hiding this comment.
That value is always
undefined
I would prefer to fix this part
There was a problem hiding this comment.
I will see what I can do
There was a problem hiding this comment.
@Keraito ah my bad. I thought you were doing !value instead.
This reverts commit cdf9bb4.
Codecov Report
@@ Coverage Diff @@
## master #3612 +/- ##
==========================================
- Coverage 40.05% 39.69% -0.37%
==========================================
Files 481 475 -6
Lines 5255 5212 -43
Branches 864 862 -2
==========================================
- Hits 2105 2069 -36
+ Misses 2632 2626 -6
+ Partials 518 517 -1Continue to review full report at Codecov.
|
The previous implementation used a ref in the input element and tried to access the value that way. That didn't work, so I removed the ref and used the |
| @@ -23,23 +23,14 @@ const Label = styled('label')({ | |||
| fontWeight: 600, | |||
| }); | |||
|
|
|||
| export default class PropField extends React.Component { | |||
| onChange = e => { | |||
| this.props.onChange(e.target.value); | |||
There was a problem hiding this comment.
You probably need to change other knob types to pick e.changes.value now
There was a problem hiding this comment.
Nevermind, this one was actually unused
|
@wuweiweiwu please review again and merge if it looks OK |
Hypnosphi
removed
the
patch:yes
|
Released as |
Issue: While doing some work on fixing an issue with the button knob discussed on Slack, I found out that the
booleanknob's behaviour was also kind of broken: after the initial toggle the value of the knob would always returnundefined. This was due to the incomingchangeobject at https://github.com/storybooks/storybook/blob/master/addons/knobs/src/registerKnobs.js#L18 always being of the form:{name: "name-of-knob", type: "boolean"}, without a value. Easiest reproducible by adding a story with a boolean knob and just printing out its direct value.What I did
Change the behaviour when the incoming
changeobject is of typeboolean, to flip the current value.