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

Storybook: Add radiocontrol component #18474

Merged
merged 1 commit into from Nov 13, 2019
Merged

Conversation

@mkaz
Copy link
Member

mkaz commented Nov 13, 2019

Description

Adds RadioControl component to Storybook
Related: #17973

How has this been tested?

Run npm run storybook:dev

Types of changes

Storybook addition.

@gziolo
gziolo approved these changes Nov 13, 2019
Copy link
Member

gziolo left a comment

We no longer can preview Storybook changes from the integration with PR.

Code wise it looks great. As a follow-up we could add a few integrations with knobs.

@mkaz mkaz merged commit 923aa53 into master Nov 13, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@mkaz mkaz deleted the add/17973-storybook-radiocontrol branch Nov 13, 2019
@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Nov 13, 2019

I debated including knobs for this one, but there wasn't really any options besides changes the text and number of options being shown. I wasn't sure how useful for understanding the component is by adding different text and/or more or less options.

@ItsJonQ or @gziolo I'd welcome your feedback on what level of knobs do you see useful in this case? Any guidance on this or similar for future components or extending this one further

The with or without help seems to be the only option, and there is another variant story for that.

@ItsJonQ

This comment has been minimized.

Copy link
Contributor

ItsJonQ commented Nov 14, 2019

@mkaz Thank you for adding this Story!

The only label I can think of would be a text knob for label.

I wish we could set up a knob that syncs better with component state. For example, if you have a knob to control the selected prop. You can change the state of the RadioControl through it 👍 . But, when the user changes it on their own (clicking), you can't update the state of the knob, so it'll go out of sync.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 14, 2019

I wish we could set up a knob that syncs better with component state. For example, if you have a knob to control the selected prop. You can change the state of the RadioControl through it 👍 . But, when the user changes it on their own (clicking), you can't update the state of the knob, so it'll go out of sync.

Yes, this is a known limitation of using useState hook which seems to take precedence with how knobs are implemented. @brentswisher tested it extensively.

@ItsJonQ or @gziolo I'd welcome your feedback on what level of knobs do you see useful in this case? Any guidance on this or similar for future components or extending this one further

I didn't mark it as a blocker because I still don't have a good sense of what we should be integrated with knobs. It feels like it should cover as much as possible, but it produces duplication when you want tot to include several examples. Thinking about it a bit more, how about we spend less time on it until we have all UI components exposed in Storybook and then revisit it by doing one pass? I have a hunch that we might also take a unified approach and do the following:

  • the default examples would provide knobs integration for all props offered with the default values provided (even if that would mean: undefined, null, false, []) - in effect the example presented would be equal to having <Example />. In some cases you would also have to configure the mandatory fields which would be also integrated with knobs
  • all other examples would be static and serve as a way to present how they can be used with no way to change them with knobs, this would make it easier to code them and developers/designers would quickly learn which story is for experiments
    What do you think?
@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Nov 14, 2019

@gziolo Thanks, I was checking in to see if I was missing something, but sounds like we're about in the same space of figuring out how knobs should operate. I like the approach to doing a pass after to get a consistent implementation, it's a good idea.

@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.