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

documentation: Clarify description of args - Add example of args controlling more than props #13978

Merged
merged 5 commits into from
Mar 2, 2021

Conversation

saschwarz
Copy link
Contributor

Issue:

There were a number of questions in the vue discord channel inquiring how to control non-props aspects of components in stories (i.e. slots). I tried to update the args text to be more encompassing and make it clear that all dynamic changes to a template go through the single args object.

What I did

  • Updated args intro
  • Added section/example for adding slots via an arg

How to test

  • 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?
    It is one 😄

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

Demo use of args outside props
@@ -98,6 +98,20 @@ Args are useful when writing stories for composite components that are assembled

<!-- prettier-ignore-end -->

## Args can modify any aspect of your components
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the prose more framework agnostic? This text gets displayed to all frameworks (only the code samples change).

Also, how would this work in React?

Copy link
Contributor Author

@saschwarz saschwarz Feb 20, 2021

Choose a reason for hiding this comment

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

I realize that, but I don't know enough about React to know how you'd use args other than as props. And that was the problem that I was trying to address: frameworks that have concepts like slots that don't exist in React (AFAIK).

The original text was very props-centric and lead to my (and others) questions about slots in discord.

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't React be done like this?

const Template = (args, { argTypes }) => (
     <page {...args}>
       {args.footer}
     </page>
);

 export const CustomFooter = Template.bind({});
 CustomFooter.args = {
   footer: <a href="https://storybook.js.org/">Built with Storybook</a>,
 };

A little sloppy, but I think that is the equivalent "args as a child" concept. How can we frame those both in this?

Copy link
Contributor Author

@saschwarz saschwarz Feb 20, 2021

Choose a reason for hiding this comment

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

If there isn't a snippet for every framework, what do I do? Do I copy the React version to the common and all the other supported frameworks?

Copy link
Member

Choose a reason for hiding this comment

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

Arg values have to be telejson serializable @phated so that example won't work. Hopefully we can mitigate that limitation with #13888

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this (simpler) example be valid for React? Do the examples have to do exactly the same thing for all frameworks?

const Template = (args, { argTypes }) => (
     <page {...args}>
       <footer>{args.footer}</footer
     </page>
);

 export const CustomFooter = Template.bind({});
 CustomFooter.args = {
   footer: 'Built with Storybook',
 };

Copy link
Member

Choose a reason for hiding this comment

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

The examples have to do approximately the same thing.

The code will necessarily be different but the technique should be similar. The code snippet for each framework can be as verbose as we need to accomplish the technique.

Copy link
Contributor Author

@saschwarz saschwarz Feb 22, 2021

Choose a reason for hiding this comment

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

@domyen If there isn't a snippet for every framework, what do I do?

  1. Do I copy the React version to the common?
  2. Copy React version to all the other supported frameworks?

Copy link
Member

Choose a reason for hiding this comment

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

@saschwarz Add snippets for Vue and React 🙏

All the other frameworks will automatically be shown React until someone contributes the code sample. There's a call to action to do this built into the snippet component already.

@jonniebigodes
Copy link
Contributor

@saschwarz i'll take a look a this tomorrow (tuesday) first thing and i'll work with you on how to properly setup the snippets accordingly. Do you mind waiting a little bit?

Stay safe

@saschwarz
Copy link
Contributor Author

@jonniebigodes Sorry I didn't see your comment until after I pushed up my changes. The snippet changes appropriately when I change to/from Vue/React/others. Please let me know any other things I need to do.

@jonniebigodes jonniebigodes added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Feb 24, 2021
Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@saschwarz thank you very much for the time and effort you put into this. Glad that you managed to solve the way the snippets are shown in the documentation. I left some items that require a bit of attention, more specifically to file naming and this should be good to go. Let me know once you've addressed the comments and we'll get this across

docs/snippets/vue/button-story-with-args.js.mdx Outdated Show resolved Hide resolved
docs/snippets/vue/page-story-slots.js.mdx Outdated Show resolved Hide resolved
docs/writing-stories/args.md Outdated Show resolved Hide resolved
docs/snippets/react/page-story-slots.js.mdx Outdated Show resolved Hide resolved
docs/snippets/react/page-story-slots.ts.mdx Outdated Show resolved Hide resolved
docs/writing-stories/args.md Outdated Show resolved Hide resolved
@saschwarz
Copy link
Contributor Author

@jonniebigodes Thank you for your patience in reviewing this PR. I merged the latest next, resolved the PR issues, and tested it. It seems OK. 😄

@jonniebigodes
Copy link
Contributor

@saschwarz on the contrary. I should be the one to thank you for your patience for not addressing the pull request changes when i told you, but i got pulled into other items that required my attention. During the weekend i'm going to take another look and if nothing stands out, we'll be good to go and i'll gladly merge this.

Do you mind waiting a little bit while i wrap up what i currently have on my hands?

It goes without saying, thank you for the time and effort you put into this and help clarify the documentation.

@jonniebigodes
Copy link
Contributor

Sorry for not addressing this earlier i was caught up on other tasks. Looks good to me. Self merging. Once again thank you for the time and effort you put into this pull request and addressing feedback so promptly.

Stay safe

@jonniebigodes jonniebigodes merged commit 2561dac into storybookjs:next Mar 2, 2021
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch vue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants