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

.addDecorator is probably not working #879

Closed
shilman opened this issue Apr 15, 2017 · 19 comments
Closed

.addDecorator is probably not working #879

shilman opened this issue Apr 15, 2017 · 19 comments

Comments

@shilman
Copy link
Member

shilman commented Apr 15, 2017

Issue by kitze
Friday Dec 23, 2016 at 23:37 GMT
Originally opened as storybook-eol/storyshots#68


I'm using material-ui in my project for some of the components. The components that are not affected by material-ui and don't need muiTheme pass the tests. But the other components that need muiTheme provided by context, are failing with a warning Warning: Failed context type: The context `muiTheme` is marked as required in `CircularProgress`, but its value is `undefined`.

Keep in mind that the stories in storybook are fine because I'm providing muiTheme through a decorator like this in my global .storybook/config.js file:

addDecorator(story => wrapWithStyles(story()));
addDecorator(story => wrapWithMui(story()));
@shilman
Copy link
Member Author

shilman commented Apr 15, 2017

Comment by arunoda
Saturday Dec 24, 2016 at 00:07 GMT


@roonyh what do you think about this issue?

@shilman
Copy link
Member Author

shilman commented Apr 15, 2017

Comment by roonyh
Tuesday Dec 27, 2016 at 05:21 GMT


@kitze I am taking a look. addDecorator works with simple wrappers but I guess something does not work with context. Have you used storyshots 2.x versions successfully with your stories?

@shilman
Copy link
Member Author

shilman commented Apr 15, 2017

Comment by fredantell
Sunday Feb 26, 2017 at 17:18 GMT


I'm running into the same issue. I have a component I specifically created for testing. I can use a global addDecorator and it works fine with storybooks. Then when I run storyshot via jest it fails trying to find functions that should have been injected via context.

If I use addDecorator on a per story level it works fine in both storybooks and storyshots.
Similarly if I just wrap each story directly in the component that also works fine for both storybook and storyshots.

@shilman
Copy link
Member Author

shilman commented Apr 15, 2017

Comment by jacobk
Friday Mar 24, 2017 at 09:46 GMT


I have this issue too with storyshots 3.2.2.

@RussMax783
Copy link

RussMax783 commented Jun 1, 2017

I have the same issue, Im using Styled Components and add the ThemeProvider to all the stories with the addDecorator. but the storyshots don't work when i use something like the withTheme component.

Any update on this? Using the v3

@tmeasday
Copy link
Member

tmeasday commented Jun 1, 2017

@RussMax783 can we get a reproduction of this? It sounds similar to #877, which I failed to reproduce. Would love to get a reproduction going!

@ktj
Copy link
Contributor

ktj commented Jun 30, 2017

Here is a reproduction:
https://github.com/ktj/storybook-rp

Run:
yarn
yarn test

@tmeasday
Copy link
Member

tmeasday commented Jul 3, 2017

Thanks @ktj that's great!

@tmeasday tmeasday self-assigned this Jul 3, 2017
@mattleff
Copy link
Contributor

I can fix the repro @ktj provided with the following diff:

image

This ensures that addDecorator is called before the stories are loaded.

@tmeasday
Copy link
Member

I'm guessing just moving the addDecorator line above the require.context line should work too, no?

This seems like a documentation problem more than anything. Perhaps the global addDecorator should log a warning if a story has already been registered, as this is unlikely to be what you want?

@mattleff
Copy link
Contributor

I'm guessing just moving the addDecorator line above the require.context line should work too, no?

Yup, that has the same affect. I'd be game for logging a warning: it would have saved me a ton of time debugging this.

@tmeasday
Copy link
Member

@mattleff keen to try sending a PR?

@mattleff
Copy link
Contributor

@tmeasday Sure, I'll try to send one in the next couple days.

@mattleff
Copy link
Contributor

@shilman Can this issue be closed now?

@tmeasday
Copy link
Member

I'll mark it merged and we'll close it when it gets released (soon!)

@usulpro
Copy link
Member

usulpro commented Sep 15, 2017

is it really solved?

@travi
Copy link

travi commented Sep 15, 2017

I don't believe so. I've still been seeing the issue for material-ui components, but haven't had the time to put together an example repo, so I haven't spoken up about it yet.

@usulpro
Copy link
Member

usulpro commented Sep 15, 2017

yep, we faced this here react-theming/storybook-addon-material-ui#64
maybe we can unite our efforts to solve this

@travi
Copy link

travi commented Sep 15, 2017

i'm happy to provide details of what we are seeing and help potential solutions, but may not have time to dig into coming up with solutions at this point. happy to provide what help i can though.

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

7 participants