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

Switch from storybook knobs to controls #966

Merged
merged 8 commits into from Oct 7, 2020
Merged

Conversation

calebeby
Copy link
Member

Overview

Closes #947

It is worth testing the behavior of all the stories I updated. I updated them by hand and there are likely places where the behavior is not exactly the way we want.

Testing

  1. Check the deploy preview, check each story that changed

@changeset-bot
Copy link

changeset-bot bot commented Sep 29, 2020

⚠️ No Changeset found

Latest commit: 7f8bef1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing to stop reminders until Netlify deploy failure has been resolved. Please request a re-review once all the status checks pass. Thanks!

@tylersticka
Copy link
Member

Some options that were using select look a little off with radios. Do we have any other options for this sort of thing?

Example before:

Screen Shot 2020-09-29 at 4 24 47 PM

And after:

Screen Shot 2020-09-29 at 4 24 22 PM

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, Caleb, this is awesome. I have some high-level questions in here, but just wanted to leave a note letting you know I'm very pleased with the progress overall.

@calebeby
Copy link
Member Author

@tylersticka I can switch radio buttons back to select. Should I switch just the ones where the empty value makes the radio button off center, or should I switch all of the radios back to selects?

@tylersticka
Copy link
Member

I can switch radio buttons back to select. Should I switch just the ones where the empty value makes the radio button off center, or should I switch all of the radios back to selects?

I haven't reviewed every control, but the ones with empty values or more than 3 or 4 values should probably use a select.

@calebeby
Copy link
Member Author

@tylersticka I went through all the radios/selects and set them based on the # of items and whether they had empty values ✅

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebeby I think we may have some options for further simplifying the definition of controls.

You can avoid associating argTypes with every individual story by assigning it to the Meta element at the beginning of the file:

<Meta title="Components/Button" argTypes={argTypes} />

You can then limit usage of argTypes on Story elements to cases where you want to override those for some reason.

We can also simplify variations by using the args property rather than modifying and passing along a unique argTypes value. Example:

// ...

<Meta title="Objects/Media" argTypes={argTypes} />

// ...

<Canvas>
  <Story name="Image">{(args) => imageDemo(args)}</Story>
</Canvas>

// ...

<Canvas>
  <Story name="Image Reversed" args={{ reverse: true }}>
    {(args) => imageDemo(args)}
  </Story>
</Canvas>

@calebeby
Copy link
Member Author

calebeby commented Oct 7, 2020

@tylersticka This is ready for another review! I switched many of the stories to use argTypes in <Meta>. Unfortunately, it seems like an individual story cannot exclude any args from the argTypes defined in <Meta>, so in the cases where I needed to do that, I left argTypes on the individual stories.

import messageDemo from './demo/default-message.twig';
import messageWithCloseDemo from './demo/message-with-close.twig';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tylersticka I removed this since I could just use the default demo and pass in the include_close arg

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. Thanks, @calebeby!

@calebeby calebeby merged commit f358510 into v-next Oct 7, 2020
@calebeby calebeby deleted the knobs-to-controls branch October 7, 2020 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from storybook knobs to controls
2 participants