Story Parameters for Official Addons

[shilman]

Story parameters has been implemented for the notes addon in #3373

We should also implement it for other official addons as part of the 4.0 release:

• info Use parameters for info addon #3697
• jest: Refactor addon-jest to use a parameter-based pattern #3678
• knobs: addon-knobs: Make withKnobs accept story parameters #3675
• viewport addon-viewport: Use the new parameterized way of decorators #3610
• backgrounds: Use parameters for backgrounds addon #3676
• options Make addon-options work with story parameters #3958 Add params-based API to options addon #3965

[mshaaban088]

viewport is implemented in #3610

[tmeasday]

Wow, that was fast, thanks @mshaaban088!

[tmeasday]

Oh. Heh, I see it was done a few days ago. Still great!

[tmeasday]

I'll have a go at the jest addon

[tmeasday]

@renaudtertrais I am looking at the jest addon and there doesn't seem like much to change. I wondered though if we could make the DX a bit easier using parameters. My suggestion would be that rather than advising people to make a withTests.js file to avoid re-importing the test file, we could instead advise the following:

// In `.storybook/config.js`
import { addDecorator } from '@storybook/react';
import { withTests } from '@storybook/addon-jest';
import results from '../.jest-test-results.json';

addDecorator(withTests({ results }));

// In a story file
storiesOf('MyComponent', module)
  .addParameters({ jest: ['MyComponent', 'MyOtherComponent'] })
  .add('This story shows test results from MyComponent.test.js and MyOtherComponent.test.js', () => (
    <div>Jest results in storybook</div>
  ));

That way we solve the problem of not wanting to import jest-test-results.json in more than one place without needing to add a new file to the project.

Let me know what you think. I am happy to implement the changes to make this work, or work with you to make it happen (we'll of course continue to support the previous API).

[Keraito]

I can work on info if nobody is already

[tmeasday]

Sounds great @Keraito

[Hypnosphi]

How about backgrounds?

[tmeasday]

Seems reasonable. Any reason you left it off the list @shilman?

[tmeasday]

How's info going @Keraito?

[Keraito]

Sorry @tmeasday, thought I had a little bit more time on my hands. I will try to get a PR rolling by the end of this week at latest.

[tmeasday]

No worries @Keraito. Just ping me if you do start it, otherwise I might do it soon to wrap this issue up.

[Keraito]

Aight, will do!

[Keraito]

@tmeasday so I started working on this, implemented the new parameter based pattern with the decorator, and am now writing some additional examples in the examples/official-storybook.

Currently, I have something like this (basically copied the first 2 stories that were already there):

storiesOf('Addons|Info.Options.parameters', module)
  .addDecorator(withInfo)
  .addParameters({
    info: {
      text: 'Component should be inlined between description and PropType table',
      inline: true, // Displays info inline vs click button to view
    }
  })
  .add(
    'Inlines component inside story', () => <BaseButton label="Button" />
  )
  .add(
    'Excludes propTypes that are in the excludedPropTypes array', 
    () => <BaseButton label="Button" />, 
    { 
      info: {
        text: 'Label propType should be excluded',
        excludedPropTypes: ['label'],
      }
    }
  );

Everything works fine, and the first story is inlined with the corresponding text. The second story however, throws away all the parameters I set in the addParameters and overrides it with the only
{ text: 'Label propType should be excluded', excludedPropTypes: ['label'] }. Is this intended? So imo it would be better if it would be merged together, particularly that the parameter per story would override any existing values set in the addParameters, and adding any non-existing entries.
Or at least put that functionality into the check between options and parameters.

[tmeasday]

@Keraito I think you are right. Perhaps you can make the change on this line:

https://github.com/storybooks/storybook/blob/fb6fb1d326acafc0411f5e1b725d98a5c0d23374/lib/core/src/client/preview/client_api.js#L110-L112

I'm not sure if we should use lodash merge to merge parameters all the way down, or just per-key? Probably just per key (so e.g. allParams.info = { ...this._globalParameters.info, ...localParameters.info, ...parameters.info })

[Hypnosphi]

Per key sounds enough

[Keraito]

@tmeasday so should I do this only for the info addon, or for all addons/keys in the 3 param objects?

[tmeasday]

@Keraito -- all keys, sorry if that was confusing ;)

[webholics]

I would like to hide the addon panel of certain pages (pages which basically just render some Markdown documentation). The way how the options plugin currently works makes this quite difficult to implement because you not only have to set showAddonPanel to false on those pages but you also have to set it back to true on all the other stories.

It would be really nice to support the options here too, like:
.addParameters({ options: { showAddonPanel: false }})

[Hypnosphi]

Yeah, addon-options is an obvious missing candidate here

[tmeasday]

Yes! Let's deprecate the setOptions(options) API and do addDecorator(withOptions(options)) instead!

(Slightly more convoluted but so much more consistent!)

[webholics]

@tmeasday what is important though is that if you do not add the withOptions decorator that some previously defined default options are picked up. Otherwise you would be forced to use the decorator in all stories. So we definitely still need the setOptions, but maybe more for setting those defaults.

[tmeasday]

Otherwise you would be forced to use the decorator in all stories.

Is this a problem? Can we not just make addDecorator(withOptions(...)) essentially identical to setOptions(...)?

You need the decorator on all stories otherwise the options story param will be ignored. Unless you have to add the decorator wherever you add the param, which would be counterproductive IMO.

At first it seems weird that a story decorator could not change the story but simply effect the UI of the manager, but if you think about it, this is what quite a few decorators do in fact (e.g. jest, notes). Perhaps its confusing, but I think a user probably isn't really thinking too hard that it's not a decorator but instead something else triggered by the story.

If I was designing it from scratch I might do it a different way, but 🤷‍♂️

[shilman]

@tmeasday Is the idea for withOptions just to simplify the API and make it more consistent (which is a good enough reason for me FWIW), or are there other benefits as well?

[tmeasday]

There is an advantage in parameter cascading (story parameters overriding whole-storybook or chapter parameters) -- see this comment: #3625 (comment)

But yeah, mostly just to be consistent.

[sapkra]

Is there any progress for the option plugin? Because this issue is the last open issue in the Storybook 4.0.0 Milestone.

[tmeasday]

I was going to look at it in the next day or two. If you are interested it could be a good contribution to make to the project?

[sapkra]

Yeah right but I have to say sorry. I definitely have not the time to contribute to this project right know.
Maybe in some month I will find some time to contribute.

[tmeasday]

No problem!

[igor-dv]

Let's remove storyshots from the list. It's a new functionality for it, and shouldn't block us.

[tmeasday]

Yay! This is now done! Great team effort everyone!

When I next have some free cycles I am going to look at #4010 but I agree its not a blocker for 4.0