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

v5 applies decorators to all the kind, no matter where it's placed #5781

Closed
Hypnosphi opened this issue Feb 27, 2019 · 11 comments
Closed

v5 applies decorators to all the kind, no matter where it's placed #5781

Hypnosphi opened this issue Feb 27, 2019 · 11 comments
Assignees
Milestone

Comments

@Hypnosphi
Copy link
Member

import React from 'react';
import { storiesOf } from '@storybook/react';
import centered from '@storybook/addon-centered/react';

storiesOf('Stories', module)
  .add('noncentered', () => 'Hello')
  .addDecorator(centered)
  .add('centered', () => 'Hello');

V4: only second story is centered
V5: both stories are centered

This should be either fixed or documented in MIGRATION.md

@tmeasday
Copy link
Member

tmeasday commented Mar 1, 2019

I'm fairly sure this was an intentional change (right @ndelangen).

Is it possible to add per-story decorators? It doesn't actually look like it is, but it would be easy to add via changing this line:

https://github.com/storybooks/storybook/blob/9f87b711b5aa9f487225c48407c2a23b7cd5df17/lib/client-api/src/client_api.js#L206

To

getDecorators: () => [...allParam.decorators, ...localDecorators, ..._globalDecorators, withSubscriptionTracking],

(Arguably we should make localDecorators and globalDecorators just be set via addParameters behind the scenes, maybe something for v5.1).

If everyone is OK with this, my MIGRATION.md notes would go something like:

Decorator changes

Previously, adding decorators to a component/kind was "stateful". If you added a decorator after a story, the decorator would only apply to future stories. For example:

storiesOf('Stories', module)
  .add('noncentered', () => 'Hello')
  .addDecorator(centered)
  .add('centered', () => 'Hello');

Moving forward, we have a new API planned that will not support this. As it is clearer, if you want to apply a decorator to subset of a kind's stories, you can use the decorator parameter;

storiesOf('Stories', module)
  .add('noncentered', () => 'Hello')
  .add('centered', () => 'Hello', { decorators: [centered] });

What does everyone think? We could also work to make the existing behaviour deprecated, but still work. I'm not sure how easy that is to do @ndelangen

@tmeasday
Copy link
Member

tmeasday commented Mar 1, 2019

I added local decorators in #5806

@shilman
Copy link
Member

shilman commented Mar 1, 2019

@tmeasday I think we should deprecate the old behavior as part of the 5.0 release. Feels "tricky" to me...

@tmeasday
Copy link
Member

tmeasday commented Mar 1, 2019 via email

@shilman
Copy link
Member

shilman commented Mar 1, 2019

Sorry, remove entirely and document rationale in MIGRATION.md

@ndelangen
Copy link
Member

storiesOf('Stories', module)
  .add('noncentered', () => 'Hello')
  .addDecorator(centered)
  .add('centered', () => 'Hello');

I think we should deprecate the old behavior as part of the 5.0 release. Feels "tricky" to me...

I tend to agree.

@shilman
Copy link
Member

shilman commented Mar 1, 2019

Yo-ho-ho!! I just released https://github.com/storybooks/storybook/releases/tag/v5.0.0-rc.8 containing PR #5806 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@tmeasday
Copy link
Member

tmeasday commented Mar 2, 2019

I had an idea -- we could make .addDecorator log a warning if it is called on a kind after a story has already been added.

I'll do the above and add the migration notes above if everyone is cool with it?

@tmeasday tmeasday reopened this Mar 2, 2019
@ndelangen
Copy link
Member

Yes perfect!

@tmeasday
Copy link
Member

tmeasday commented Mar 2, 2019

=> #5819

@shilman
Copy link
Member

shilman commented Mar 3, 2019

w00t!! I just released https://github.com/storybooks/storybook/releases/tag/v5.0.0-rc.9 containing PR #5819 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Mar 3, 2019
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

4 participants