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

Allow more concise way of defining stories with args #12654

Closed
pksunkara opened this issue Oct 3, 2020 · 25 comments
Closed

Allow more concise way of defining stories with args #12654

pksunkara opened this issue Oct 3, 2020 · 25 comments

Comments

@pksunkara
Copy link
Member

pksunkara commented Oct 3, 2020

Problem

Currently, when I am using args, I would write something like the following:

import React from 'react';
import Button from './Button';

export default {
  title: 'Button',
  component: Button,
};

const Template = (args) => <Button {...args}></Button>;

export const Primary = Template.bind({});

Primary.args = {
  color: 'primary',
};

export const Secondary = Template.bind({});

Secondary.args = {
  color: 'secondary',
};

I feel like there are quite a few extra repetitions that can be magicked away.

Proposal

I think we can allow the user to write something like the following:

import React from 'react';
import Button from './Button';

export default {
  title: 'Button',
  component: Button,
  template: (args) => <Button {...args}></Button>,
};

export const Primary = {
  args: {
    color: 'primary',
  },
};

export const Secondary = {
  args: {
    color: 'secondary',
  },
};

Basically, we just add some more magic before building the stories.

if (typeof story !== 'function') {
  story = Object.assign(storyGroup.default.template.bind({}), story);
}

Even more magic

In fact, for some frameworks (react for sure, would need to investigate others), we can do even more magic. If the template for the component is simple, we can allow users to skip it.

// Modifying the above code
export default {
  title: 'Button',
  component: Button,
};

The magic we would add would be something like the following:

storyGroup.default.template ||= (args) => storyGroup.default.component(args);

Cons

Please correct me if I'm wrong, but the only issue I foresee is not being able to reuse a story directly in other component's stories. But I think if we put our minds together, we can solve it 😄

Comments

@ndelangen This is what I was talking about yesterday. Since Docs does show properly how to use the story with JSX, I feel like this args pattern doesn't have any other cons other than being repetitive which I am trying to solve here.

@shilman If accepted, do you think we can do this in 6.1.0?

@pksunkara
Copy link
Member Author

Updated the main post with a disadvantage of the proposal.

@shilman
Copy link
Member

shilman commented Oct 3, 2020

@pksunkara I like it. The args construct was designed as a fully backwards-compatible stepping stone to a fully declarative "stories as data" approach in the same vein as what you've proposed. We were discussing something more along the lines of:

export const Primary = {
 color: 'primary',
};

But I like your version better because it's preserves the "story function" concept and makes template first class. So big thumbs up to this proposal.

The reason we didn't take the extra step in 6.0, and also why I hesitate about this is because we still haven't figured out how to make this kind of CSF portable.

Please take a look at the original CSF post and then take a look at this discussion and maybe this PR.

I think if we can come to some agreement about the best way to make this compatible with external tools, using testing tools as a discussion driver, we can incorporate this change in 6.x.

We're also going to add asynchronous loading in 6.1, so we need to make sure this is compatible with that, but I'm less worried about that since this maps very cleanly onto the existing structure.

cc @tmeasday

@shilman shilman added the csf label Oct 3, 2020
@jonspalmer
Copy link
Contributor

I'd be curious on how we map these example to the JSON story format we use for server. That's been a pretty good metric for the portability of args (admittedly with some restrictions)

@tmeasday
Copy link
Member

tmeasday commented Oct 5, 2020

Thanks for writing this up @pksunkara. Yeah, we've talked about various versions of this and I kind of agree this is probably the most incremental and easiest to understand based on where we are now.

As @shilman alluded to, we've want to slowly reach this change, as this is sort of stepping over a rubicon from story exports being a really simple "function that returns something renderable" into something a bit more magic. To be a bit clearer, the downsides are:

  1. You can't just import a story from a CSF file and use it elsewhere, where "elsewhere" might be:
    a. Inside another story
    b. In somewhere else like a unit test
    c. In an external tool.

We can definitely make a renderStory export to make it easier to reuse them, but it does sort of betray one of the aims of CSF. So that's a big downside.

OTOH, the "export is renderable" is kind of tricky thing anyway as some stories require decorators from e.g. .storybook/preview.js or even addons to render properly, which sort of stuff things up anyway.

  1. It's "magic" and anytime there's magic it makes it hard for new users to understand what's going on. Maybe it's not so hard though.

An alternative idea we've kicked around would be to make it possible to build a story generator:

import React from 'react';
import { makeStoryTemplate } from '@storybook/react'; // <- this is react specific
import Button from './Button';

export default {
  title: 'Button',
  component: Button,
};

const makeStory = makeStoryTemplate(Button);

export const Primary = makeStory({
  args: {
    color: 'primary',
  },
});

export const Secondary = makeStory({
  args: {
    color: 'secondary',
  },
});

The definition of makeStoryTemplate is super simple, if you like this you could try it now:

const makeStoryTemplate = (Component) = (args) => <Component {...args} />;

It might get a bit more complicated if async loading pans out the way we are thinking.


@jonspalmer

I'd be curious on how we map these example to the JSON story format we use for server. That's been a pretty good metric for the portability of args (admittedly with some restrictions)

I think the package that consumes the JSON would definite how to generate story template "functions" which make stories from story data. In your case the template doesn't do much IIRC, so it would be really simple.

@pksunkara
Copy link
Member Author

I agree with the scenarios you mentioned.

the "export is renderable" is kind of tricky thing anyway as some stories require decorators from e.g. .storybook/preview.js or even addons to render properly, which sort of stuff things up anyway.

Yeah, I was thinking about this during the weekend. But the scenario of "Inside another story" would not be an issue here, right? It's only the other 2 scenarios


I do like the idea you proposed regarding makeStory. The concerns I would have is:

  1. How would you create the makeStory from a custom template function instead of the simple one?
  2. We wanted to move away from using APIs while writing stories.

@shilman shilman added the PN label Oct 5, 2020
@tmeasday
Copy link
Member

tmeasday commented Oct 6, 2020

the "export is renderable" is kind of tricky thing anyway as some stories require decorators from e.g. .storybook/preview.js or even addons to render properly, which sort of stuff things up anyway.

Yeah, I was thinking about this during the weekend. But the scenario of "Inside another story" would not be an issue here, right? It's only the other 2 scenarios

Correct, yeah. Although component-level (or story-level) decorators don't get applied if you just import a story and run it, like in https://storybook.js.org/docs/react/workflows/stories-for-multiple-components#reusing-subcomponent-stories

@pksunkara
Copy link
Member Author

if (typeof story !== 'function') {
  story = Object.assign(storyGroup.default.template.bind({}), story);
}

With the above part of the code I mentioned, we will be giving users the choice of either defining a full story or just the data.

I think we can go ahead and add support for the data, and if the users want to re-export the story, they still have the option to make it a full story.

But as you said, the component and story level decorators don't get applied and the other scenarios are still not solved. To solve this, the users can currently use renderStory, right?

To me, it looks like that all non-default exports of .stories.js file should be wrapped with renderStory. So, why not do it at the webpack level?

import { Unchecked } from './ListItem.stories';

export const OneItem = (args) => (
  <List {...args}>
    <Unchecked {...Unchecked.args} />
  </List>
);

Basically, what webpack config will do is wrap Unchecked in an renderStory. And this can be expanded to the other utilities too because they will still import from file name ListItem.stories

@tmeasday
Copy link
Member

So, why not do it at the webpack level?

Probably a babel plugin would be more flexible to be used in other places. Maybe we give up on X.stories.js being directly importable in any but the simples cases, and instead X.stories-compiled.js (after the babel plugin) being the thing that external tools use.

Thoughts on that @shilman?

@adamszeptycki
Copy link

adamszeptycki commented Oct 14, 2020

I just started using new Storybook CSF, and I like it's cleaner but I cannot generate stories.

Usually our components take limited amount of props. I would like to be able to enumarate them and generate story
Proposal

import React from 'react';
import Button from './Button';

//defined here only to make example more understandable
interface ButtonProps {
 color: 'primary' | 'secondary'
 text:  string
 size: 'small' | 'medium' | 'big'
}


const stories = []

['primary' , 'secondary'].forEach(color => {
    ['small', 'medium', 'big'].forEach(size) => {
        return stories.append({
            args: {color:color, size: size, text: "Click Me"}
            title: `color ${color} size: ${size}"
        })
    }
})

export {stories}

export default {
  title: 'Button',
  component: Button,
  template: (args) => <Button {...args}></Button>,
};

@shilman
Copy link
Member

shilman commented Oct 19, 2020

@adamszeptycki We're not ready to support dynamic stories yet and it's a separate discussion. The workaround you can do today is provide a custom loader that outputs static CSF at load time. The reason we force CSF to be statically analyzable is because we want Storybook and third-party tools to be able to process CSF by analyzing the source code/AST without having to actually execute the code. The moment we break that, it will be very hard to walk that back.

Why do we care? For example, in order to support the storiesOf format in Composition we have to install Puppeteer since many stories require a browser environment to evaluate. If stories are statically analyzable, this kind of dependency goes away and we open up a bunch of new doors.

@shilman
Copy link
Member

shilman commented Oct 19, 2020

@tmeasday @pksunkara I love this proposal but I don't think we're ready to ship it publicly in 6.1. I'd be open to shipping an experimental loader that implements this so we can try it out before we commit to supporting it in CSF.

@pksunkara
Copy link
Member Author

@shilman Sounds good to me. A phased approach is best here. I am glad we found a better way to represent stories as purely data. 😄

@tmeasday
Copy link
Member

experimental loader

Would a babel plugin be more general than a loader? Apart from that I'm 👍

@pksunkara
Copy link
Member Author

@shilman Where do we want this on our roadmap?

@shilman
Copy link
Member

shilman commented Jan 2, 2021

@pksunkara sorry for the slow response. I think we can add this feature quietly whenever you (or somebody else) is ready to add it. I'm with @tmeasday on implementing it as a babel plugin.

We've made a lot of big changes: CSF in 9/2019, Args in 8/2020. I've gotten a lot of feedback that Storybook is changing too fast. Our improved documentation should mitigate that, but doesn't fundamentally solve it.

Here are are the preconditions I'd like to see before we actually promote this as a new way to write stories:

  • Testing utilities released and stable (ETA 6.2?)
  • Args and ArgTypes are well-documented (ETA 6.2)
  • Args and Controls bugs/missing features are mostly fixed (ETA 6.2)
  • Args-based Source snippet generation stable (ETA 6.3?)

If all of this stuff feels stable and mature and the new format plays nicely with these different aspects of Storybook, I think we can document/market the new format. So release in 6.2, market in 6.3 at earliest?

@wKich
Copy link
Member

wKich commented Jan 8, 2021

Hi everyone. In my last company, we wrote a special component, that accepts in props another component and prop combinations. That component renders a combinations grid. And this is how it looks (it also supports autocomplete and typechecking):

const sizeStates = getProps('size', ['small', 'medium', 'large']);
const arrowStates = getProps('arrow', [true, 'left']);
const useStates = getProps('use', ['default', 'primary', 'danger', 'pay', 'link']);
const widthStates = getProps('width', [100, 'auto']);

export const DefaultCombinations = () => (
  <ComponentCombinator
    Component={Button}
    presetProps={{ children: 'Button' }}
    combinations={[useStates, sizeStates, arrowStates, widthStates]}
  />
);

You can see implementation here

I like a more declarative approach that could be transformed into compile-time stories and could be statically analyzable.

I can suggest this syntax. We already have the component option, so we just need to write combinations.

export default {
  title: 'Button',
  component: Button,
  combinations: {
    size: ['small', 'medium', 'large'],
    color: ['primary' , 'secondary'],
  },
}

Or it could be

export default {
  title: 'Button',
  component: Button,
  parameters: {
    combinations: { /* ... */ },
  },
}

We can allow user pass array as well for more controllable combinations

combinations: [
  { size: 'small', color: 'primary' },
  { size: 'large', color: 'secondary' },
],

The addon could handle these combinations, which compiles them to regular stories and one special story with a table or a list of all combinations.

We can parameterize how composed story should look like. And we can fully support autocomplete and type checking for combinations. We can generate combinations using pairwise testing technic, which reduces the number of combinations and still provides a high test coverage rate.

If you like the idea, I can give it a try to do PoC.

@tmeasday
Copy link
Member

@wKich -- one question I have about this feature -- how crucial is the creation of a whole bunch of stories here? (as opposed to the grid feature)

I guess I am asking because something that seems more constrained in scope (and maybe even possible to implement in a decorator/addon right now) is something that takes a story and renders it several times in a grid with different arg values.

I could imagine something like:

export const Combinations = Template.bind({})
Combinations.parameters = {
   argCombinations: {
    size: ['small', 'medium', 'large'],
    // etc
   }
};

// Then in a decorator
const createGrid = (Story, context) => {
  const argSets = getCombinations(context.parameters.argCombinations)

  // Forgive my psuedo code here
  return <table> {argsSets.map((args) => <Story {args} />} </table>
}

I'm not quite sure about the <Story {args} /> bit, but we could look into making that work if it doesn't already.

I have some ideas about how a "combinations" addon could work if the story also has a set of args defined, or if you change arg values via controls. I think it actually could be really cool -- combinations would be like a separate panel, alongside the canvas.

@tmeasday
Copy link
Member

tmeasday commented Jan 11, 2021

@pksunkara one other thing that I didn't mention in the above thread (I don't think), is an idea of @ghengeveld -- you can actually do cool things with the "last" decorator in terms of mapping arbitrary objects to rendered components.

So you could do something like:

export default {
  component: Button,
  decorators: [(args) => <Button {...args} />)]
}

export const Primary = {
  color: 'primary',
};

I'm not sure but the decorator could even be more general than that:

// In preview.js

export const decorators = [
  (storyOrArgs, { component: Component } ) => typeof storyOrArgs === 'function' ? storyOrArgs() : <Component {...storyOrArgs} />,
];

Does that work?

@wKich
Copy link
Member

wKich commented Jan 11, 2021

@tmeasday, I think it could be more than one use case for combinations.

  • First, is when you want to reduce boilerplate code in your stories (from the first comment)
  • Second, is when you have a basic component with a big amount of possible props
  • Third, is maybe you want to go from a grid to an individual story to play a little bit with controls (because you can't do it in a grid) So stories in a grid should be referenceable.

I like the idea to place the grid in a separate panel.

BTW, argsCombinations on the story level is more flexible

@pksunkara
Copy link
Member Author

// In preview.js

export const decorators = [
  (storyOrArgs, { component: Component } ) => typeof storyOrArgs === 'function' ? storyOrArgs() : <Component {...storyOrArgs} />,
];

I guess your question is more about whether the template is needed to be even defined in the first place, right? If yes, yeah it looks like this helps.

But the following will still be an issue if we don't implement the babel plugin/loader.

Although component-level (or story-level) decorators don't get applied if you just import a story and run it, like in storybook.js.org/docs/react/workflows/stories-for-multiple-components#reusing-subcomponent-stories

@tmeasday
Copy link
Member

Although component-level (or story-level) decorators don't get applied if you just import a story and run it, like in storybook.js.org/docs/react/workflows/stories-for-multiple-components#reusing-subcomponent-stories

Yes, although this is a problem we need to address. Good news is that @yannbf is looking at it already in #12959

@pksunkara
Copy link
Member Author

@shilman @tmeasday Now that CSF3.0 is ready to go out, the only thing left from this is:

Probably a babel plugin would be more flexible to be used in other places. Maybe we give up on X.stories.js being directly importable in any but the simples cases, and instead X.stories-compiled.js (after the babel plugin) being the thing that external tools use.

Do you guys agree?

Currently, I am unable to do the following https://github.com/storybookjs/testing-react#composestories with CSF 3.0

@shilman
Copy link
Member

shilman commented Oct 11, 2021

@pksunkara Please join the discussion going on over here #15954

@shilman
Copy link
Member

shilman commented Oct 11, 2021

Between what's already there in CSF3 and the various follow-up issues, I think most of the spirit of this proposal is either ready or continuing elsewhere, so I'm going to close this issue. @pksunkara thanks so much for kickstarting this -- I checked and saw that I forgot to add your name to the CSF3 blog post, so I've updated that just now.

@shilman shilman closed this as completed Oct 11, 2021
@pksunkara
Copy link
Member Author

No worries. 😄

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

No branches or pull requests

6 participants