Controls/Knobs: Set input to empty string when value is null

[kmurgic]

Issue: #13826

What I did

React is adding errors to the console: value prop on 'input' should not be null and A component is changing a controlled input of type number to be uncontrolled whenever the contents of a number control or knob are deleted.

In this PR, I set the input value to an empty string when the value of the knob or control is null. This prevents React from throwing errors about input values being null and inputs changing from controlled to uncontrolled.

How to test

In the current official storybook and kitchen sink examples if you delete the text inside a number knob or control there will be errors in the console. Examples of this issue can be found in the stories for examples/official-storybook in controls/number and addons/knobs/withKnobs/tweaks static values.

In this branch there are no longer any errors in the console when you delete the text in a number control or number knob.

• Is this testable with Jest or Chromatic screenshots?
  No

• Does this need a new example in the kitchen sink apps?
  No

• Does this need an update to the documentation?
  No

If your answer is yes to any of these, please make sure to include it in your PR.

[ghengeveld]

I wonder if we should instead handle this in the story store, so that any other "consumers" of these args will gracefully handle null as well. I'm thinking mapArgsToTypes but applied to any arg, not just URL args.

[ghengeveld]

Was this intentional?

[kmurgic]

No, not sure how this snuck in there.

[ghengeveld]

I would just inline this variable.

[kmurgic]

Okay, I can do that. I was on the fence about whether or not to inline it.

[tmeasday]

In 6.3 we are going to update all the controls to allow an "unset" value -- i.e. undefined. This is to explicitly allow non-required args to be unset (especially the initial value of the arg).

With that in mind, it shouldn't be possible to have null for a string-typed arg, anywhere in the stack. We'd also need to handle the undefined case when rendering the input I guess.

So I am not sure if this PR is a step towards that or not, WDYT @kmurgic?

[kmurgic]

I wonder if we should instead handle this in the story store, so that any other "consumers" of these args will gracefully handle null as well. I'm thinking mapArgsToTypes but applied to any arg, not just URL args.

I'm not sure if I am interpreting this comment correctly. When a control or knob is cleared, I would think null - or possibly undefined - makes the most sense for the value of the knob/control. We only want the empty string value passed to the actual input in the knobs panel or controls panel to avoid the input inside the panel changing between controlled and uncontrolled. This way we avoid making a breaking change here.

I personally like having the value of a knob be undefined when the last character in the input for a number knob/control is deleted. For example, in an InputField component I have an optional maxLength property that I am using a number control for. It would make sense to me that when the control panel input is cleared, the maxLength property would be undefined. In my code now I am doing const maxLengthProp = maxLength === null ? undefined : maxLength in order to not pass maxLength when that control is cleared. I think similar use cases are at least relatively common. However, this is a breaking change and not a bug fix, and I think that change falls outside of the scope of this PR.

[kmurgic]

In 6.3 we are going to update all the controls to allow an "unset" value -- i.e. undefined. This is to explicitly allow non-required args to be unset (especially the initial value of the arg).

With that in mind, it shouldn't be possible to have null for a string-typed arg, anywhere in the stack. We'd also need to handle the undefined case when rendering the input I guess.

So I am not sure if this PR is a step towards that or not, WDYT @kmurgic?

It should be safe to do value={value === null || value === undefined ? '' : value} to make sure that undefined values don't trigger this error either. If Storybook is planning on allowing text knobs/controls to be undefined then this change likely needs to happen in text controls/knobs as well. I'm fairly confident that there is no reason why the value passed to the input inside the controls or knobs panel should ever be null or undefined, even if the value of the knob/control itself is null or undefined.

[kmurgic]

Apologies for taking such a long break from this PR.

[tmeasday]

Hi @kmurgic thanks for coming back!

Your timing is actually great, we are working on the solution I mentioned this week: #14899

So to address your points:

I'm fairly confident that there is no reason why the value passed to the input inside the controls or knobs panel should ever be null or undefined, even if the value of the knob/control itself is null or undefined.

In that PR we show a button rather than the control if the value of the arg is undefined (null shouldn't be possible for a e.g. number-valued arg[1]). So this concern is moot I suppose.

I personally like having the value of a knob be undefined when the last character in the input for a number knob/control is deleted.

We are tossing up with this and setting it 0. There are a few controls where this is relevant:

1. Text control -- this one is easy, when you clear it, it should absolutely go to ''.
2. Number control -- we are thinking we'll set it to 0.
3. JSON object control -- we are thinking we'll set it to {}

There are couple reasons that setting the arg to undefined is a bit problematic:
a. Inconsistency between types -- it isn't possible to set a text arg to undefined without totally resetting all the args in the story. If we make it possible for a number arg, people are going to be confused.
b. Inconsistency in UI -- initially an undefined arg shows a button. If you start editing it, then clear it, it looks different (an empty control) to what it did before (a button). Will that be confusing to users?

We don't love setting it to a "random" value in 2-3 either, arguably that's an inconsistent UI too. We were thinking that if you blurred the control we might surface the new value (i.e. you clear the control, tab away, and it changes to "0").

[1] I'm actually unsure what happens if you mis-type an arg; for instance if you set a number-value arg to an object; what gets passed to the control? What should we do? We should probably address that now.

[kmurgic]

In 6.3 we are going to update all the controls to allow an "unset" value -- i.e. undefined. This is to explicitly allow non-required args to be unset (especially the initial value of the arg).

I was having trouble deciphering the changes to figure out how this works. Can I "unset" a number input after I set it? For example if I set a control for my maxLength property, is there a way to remove the maxLength property entirely so that I'm passing maxLength: undefined to the component? What would I click to do that?

Number control -- we are thinking we'll set it to 0.

As long as there is a reasonable way to unset the number control I think this solution seems good to me.

The thing I would want to avoid is not giving users any way to set a value for a number control to undefined. It's nice right now in SB that I can delete the contents of a number input and have the prop become null. For some optional props a value of undefined or null is very different from a value of 0. Examples maxLength?: number, yearFounded?: number, lattitude?: number.

Really though in a way this discussion is out of the scope of the issue/bug. In terms of the bug the important thing is that if we are ever showing an input in the controls section when the value of a knob is null or undefined we should be setting the value of the input that is inside the control panel to an empty string in order to avoid React's warning about a component switching between controlled and uncontrolled. Although if we are sure that if the knob value is null or undefined we won't even be showing an input component, then I suppose that solves this as well.

[kmurgic]

[1] I'm actually unsure what happens if you mis-type an arg; for instance if you set a number-value arg to an object; what gets passed to the control? What should we do? We should probably address that now.

I'm not an expert here by any means, but as a user my preference would be to have an error in the console and probably set the control as either unset or whatever it's default empty value would be. It also doesn't seem that unreasonable to me if the story failed to build in this case - as long as we could make it very clear to the user why the build failed.

[tmeasday]

The thing I would want to avoid is not giving users any way to set a value for a number control to undefined. It's nice right now in SB that I can delete the contents of a number input and have the prop become null. For some optional props a value of undefined or null is very different from a value of 0. Examples maxLength?: number, yearFounded?: number, lattitude?: number.

As of the latest alphas you can unset a control via resetting all the controls for the story. @domyen and I discussed this a couple times, and decide that the inconsistency (for a number control you can unset it by clearing, for a text control you cannot) was probably worse than the inconvenience of not being able to unset a control without clearing all.

Perhaps users will feel differently once they have a chance to play with this.

I'm not an expert here by any means, but as a user my preference would be to have an error in the console and probably set the control as either unset or whatever it's default empty value would be. It also doesn't seem that unreasonable to me if the story failed to build in this case - as long as we could make it very clear to the user why the build failed.

I think that probably makes sense. I suspect what we'll do is pass the arg through (the arg type could be wrong too), but the control will either not work or be unset as you say. In any case, if you want to discuss if further we have a specific issue for this:

#14926

---

Really though in a way this discussion is out of the scope of the issue/bug. In terms of the bug the important thing is that if we are ever showing an input in the controls section when the value of a knob is null or undefined we should be setting the value of the input that is inside the control panel to an empty string in order to avoid React's warning about a component switching between controlled and uncontrolled.

I think we are good here; if you can still repro this issue in the latest alphas, let us know.