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

No decorators in storyshots #1849

Closed

Conversation

usulpro
Copy link
Member

@usulpro usulpro commented Sep 15, 2017

Issue:

What I did

How to test

Is this testable with jest or storyshots?

Does this need a new example in the kitchen sink apps?

Does this need an update to the documentation?

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

@usulpro
Copy link
Member Author

usulpro commented Sep 15, 2017

cc @coreylight

This reverts commit 49d97ba, reversing
changes made to 7b56132.
@coreylight
Copy link

Thanks @usulpro!

}
}
>
Array [
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is NOT what we want to happen. Part of the actual story is gone from the snapshot!

@ndelangen
Copy link
Member

Should we keep a list of all the decorators and only filter out those components?

We should also not just remove the first parent, but we'd have to recursively remove/filter them from top, since decorators could be wrapped (multiple decorators around 1 story).

@coreylight
Copy link

@ndelangen I like those suggestions. I'm not very familiar with the codebase so I fully expected some iteration on this 😊

@danielduan
Copy link
Member

I'm gonna close this PR since there hasn't been much activity here. Feel free to reopen or leave a comment here so we can reopen it for you.

@axelnormand
Copy link

axelnormand commented Mar 9, 2018

I'm still very keen on not seeing decorators in the storyshot as I use enzyme shallow so the storyshot becomes quite useless and it doesn't show the component in question :)
Hopefully I can pick up this PR and fix it if i have time

@axelnormand
Copy link

axelnormand commented Mar 9, 2018

In the meantime my plan is to use enzyme shallow .dive() in my storyshots.test.ts file to recurse down to find story in question and use that in the call to toMatchSpecificSnapshot

@ndelangen
Copy link
Member

@axelnormand agreed

Join us on slack and we'll collaborate and make it happen, I think @Hypnosphi would love this as well!

@ggarek
Copy link
Contributor

ggarek commented Jan 15, 2019

Guys, is there any resolution to this issue? @ndelangen @axelnormand

I am having same issue: i have my decorators in all the storyshots.

I was using "@storybook/addon-storyshots": "^3.4.7" and it seem not to be an issue.
Then i upgraded to "@storybook/addon-storyshots": "^4.1.6" (current latest) and i hit the problem.

I found the issue #1620, which is closed for being obsolete with no resolution.
Then i have found this PR, and do not see any leads here as well.

@axelnormand
Copy link

Alas i didn't have time (yet) to contribute anything to the storybook project which might help this.

What i ended up doing was mocking the addDecorator method in my jest storyshot tests so it just renders the story (ie does nothing), therefore no decorator appears in the snapshot

// mock addDecorator so they are not in snapshot
jest.mock('@storybook/react-native', () => {
  const actual = require.requireActual('@storybook/react-native');
  return {
    ...actual,
    storiesOf: (...args: any[]) => {
      const storybook = actual.storiesOf(...args);
      storybook.addDecorator = () => storybook;
      return storybook;
    },
    addDecorator: () => null,
  };
});

@ndelangen
Copy link
Member

That's an interesting solution @axelnormand.

In 5.0.0 I added new api's for storyshots to be able to ACCESS the original storyFn and a fully decorated Fn.

It's currently using the fully decorated Fn always, but it could fairly easily start rendering the raw one based on an option/parameter.

I think that's really the way forward, since some people will WANT the decorators and some DON'T. And quite possibly it might be different per story!

@ndelangen
Copy link
Member

I might take a crack at it.

@axelnormand
Copy link

oh didnt know that about the new 5 API, great stuff. Thanks @ndelangen

@ndelangen
Copy link
Member

@axelnormand we cleanup up A LOT of internals, making us better able to make changes like this. On top of that the addParameters api has really given a robust way of configuring anything on a global or story level.

@ndelangen
Copy link
Member

Gonna open a PR in a few minutes adding this feature!

@aralroca
Copy link

@axelnormand very great workaround. However, I have some decorators that are adding Context Providers to the stories... Without it, stories don't consume correctly the data... So I need to mock also all these Context...? 😯 Or maybe there is a better way to add these Context Providers than using decorators?

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

Successfully merging this pull request may close these issues.

7 participants