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

addon-knobs select should be able to have undefined options. #4487

Closed
lifeiscontent opened this issue Oct 18, 2018 · 22 comments · Fixed by storybookjs/telejson#19
Closed

addon-knobs select should be able to have undefined options. #4487

lifeiscontent opened this issue Oct 18, 2018 · 22 comments · Fixed by storybookjs/telejson#19

Comments

@lifeiscontent
Copy link
Member

If you are reporting a bug or requesting support, start here:

Bug or support request summary

    const pressed = select(
      'Pressed',
      { Undefined: undefined, False: false, True: true },
      undefined
    );

I'd expect there to be a value called Undefined which is selectable, but it is removed when rendered in the knobs panel.

Please specify which version of Storybook and optionally any affected addons that you're running

  • @storybook/react latest
  • @storybook/addon-knobs latest (alpha)

Affected platforms

  • If UI related, please indicate browser, OS, and version
  • If dependency related, please include relevant version numbers
  • If developer tooling related, please include the platform information

Screenshots / Screencast / Code Snippets (Optional)

    const pressed = select(
      'Pressed',
      { Undefined: undefined, False: false, True: true },
      undefined
    );
@bennycode
Copy link

Supporting undefined options is very important when displaying React components which have been written in TypeScript.

In TypeScript it is common sense to use the question mark to declare optional parameters and properties. So an interface for a component's properties can look like this:

interface Props {
  environment?: 'linux' | 'mac' | 'win';
}

Which is equivalent to:

interface Props {
  environment: 'linux' | 'mac' | 'win' | undefined;
}

But different from:

interface Props {
  environment: 'linux' | 'mac' | 'win' | null;
}

Unfortunately, it is not possible to use undefined values with select, so states with optional props cannot be shown with @storybook/addon-knobs. 😢

@stale stale bot added the inactive label Nov 14, 2018
@storybookjs storybookjs deleted a comment from stale bot Nov 14, 2018
@stale stale bot removed the inactive label Nov 14, 2018
@lifeiscontent
Copy link
Member Author

lifeiscontent commented Nov 15, 2018 via email

@igor-dv
Copy link
Member

igor-dv commented Nov 16, 2018

@lifeiscontent , sorry for the delay =) you can chat with us on Discord:

@kallaugher
Copy link

@lifeiscontent Are you still working on this? If not, I'll start looking into it 🙂

@Hypnosphi
Copy link
Member

Looks like the root of the issue is that telejson library that we use to transmit data between preview and manager frames uses JSON.parse(data, reviver()) which ignores undefined values:

If the reviver function returns undefined (or returns no value, for example, if execution falls off the end of the function), the property is deleted from the object.

@ndelangen you probably need to iterate on object fields manually instead of using reviver parameter of JSON.parse.

@alfnielsen
Copy link

alfnielsen commented Apr 24, 2019

Hey,
Just to add a little extra info to @bennyn comments, it's not just TypeScript but ES6 (EcmaScript 2015) that uses optional parameters which only accepts undefined (not null).

In TypeScript it would be something like this:

interface Props {
  environment?: 'linux' | 'mac' | 'win';
}
const myComponent = ({ environment = 'win' }: props ) => { (...) }

but you can do the same in ES6 like this:

const myComponent = ({ environment = 'win' }) => { (...) }

if environment = null it will be null instead the method and not the default value 'win'

@ndelangen ndelangen added this to the 5.2.0 milestone Jun 3, 2019
@christophehurpeau
Copy link
Contributor

I can create an PR on friday if no one works on this yet ?

@Hypnosphi
Copy link
Member

Sure @christophehurpeau

@Hypnosphi
Copy link
Member

Hypnosphi commented Jul 1, 2019

@christophehurpeau note that it's telejson library that needs a PR:

#4487 (comment)
storybookjs/telejson#7

@christophehurpeau
Copy link
Contributor

I can't see how we could fix this in telejson, what about transorming undefined to 'undefined' before stringify ?

@lifeiscontent
Copy link
Member Author

honestly, I don't think telejson is the right starting point for this work, what about a reviver and replacer for the parse/stringify for the knobs?

@Hypnosphi
Copy link
Member

@lifeiscontent what makes you think so?

@lifeiscontent
Copy link
Member Author

Well, I guess it would depend on if telejson is only being used for knobs. Considering this is a requirement of just knobs currently it makes sense that it should live there, unless telejson is purposely built for knobs. If we later find we want the same functionality across another part of the platform it should then be revisited.

Sent with GitHawk

@Hypnosphi
Copy link
Member

@lifeiscontent telejson states in the repo description that it supports undefined while in fact it doesn't. So it's not some specific requirement of knobs

JSON parse & stringify with support for cyclic objects, functions, dates, regex, infinity, undefined, null, NaN, Classes, Instances

@lifeiscontent
Copy link
Member Author

@lifeiscontent telejson states in the repo description that it supports undefined while in fact it doesn't. So it's not some specific requirement of knobs ...

@Hypnosphi okay, then I guess it makes sense to do there. Are you working on this? Or should I open a PR?

Sent with GitHawk

@Hypnosphi
Copy link
Member

Are you working on this?

No I'm not. Feel free to open a PR

@shilman
Copy link
Member

shilman commented Sep 27, 2019

We probably need to upgrade telejson?

@shilman shilman reopened this Sep 27, 2019
@ndelangen
Copy link
Member

Yeah I'll ship a new version soon.

@Steven-Gassert
Copy link

@ndelangen has a new version been shipped with this functionality yet? If not, is it possible to get a link somewhere this work is being tracked?

@ndelangen
Copy link
Member

@shilman
Copy link
Member

shilman commented Nov 26, 2019

Released in https://github.com/storybookjs/storybook/releases/tag/v5.3.0-beta.8

Closing this. Please holler if this didn't fix the problem

@lifeiscontent
Copy link
Member Author

@shilman still an issue with args.
Screen Shot 2020-11-24 at 4 41 54 PM

Screen Shot 2020-11-24 at 4 43 19 PM

should have an option here to not have a value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants