-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Storyshots: First-class CSF support #8000
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-storyshots-csf.storybook.now.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great @Hypnosphi 💯
I had a few minor changes inline -- should be an easy fix.
Also, I'm wondering if you've seen this PR that's vaguely related: #7330
I haven't had a chance to address it yet, but I'm wondering if it influences this PR at all? (It might not, just asking.)
addons/storyshots/storyshots-core/src/api/snapshotsTestsTemplate.js
Outdated
Show resolved
Hide resolved
addons/storyshots/storyshots-core/stories/required_with_context/Button.stories.js
Outdated
Show resolved
Hide resolved
@Hypnosphi are there tests to make sure it still works well with the storiesOf files? |
Ok, I'll revert some of the stories |
# Conflicts: # addons/storyshots/storyshots-core/package.json
Merged storybookjs/require-context.macro#13 and released as v1.2.0 |
@kylemh anything required on the storybook side, or is that just an FYI? AFAIK it's not a storybook dep |
@shilman just an FYI! Nothing needed more from us. Note Hypno's 3rd bullet in OP. |
@kylemh @Hypnosphi thanks for the clarification. i know about the package -- just missed the ref in your original PR @Hypnosphi . makes sense! 👍 |
# Conflicts: # addons/storyshots/storyshots-core/package.json
# Conflicts: # examples/preact-kitchen-sink/src/stories/__snapshots__/welcome.stories.storyshot
@shilman can you please re-review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work @Hypnosphi. Thanks so much for taking care of this 🙏
@Hypnosphi can you take a look at the failing story in Chromatic? |
@shilman looks like at some point @ndelangen switched chromatic to use production build, and in production webpack replaces filenames with numbers. It won't be a problem in storyshot tests. I think I'll filter out the |
# Conflicts: # addons/storyshots/storyshots-core/package.json
Storyshots: First-class CSF support
// jest.config.js | ||
module.exports = { | ||
transform: { | ||
'^.+\\.stories\\.jsx?$': '@storybook/addon-storyshots/injectFileName', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation change differs from what is displayed in the warning below. https://github.com/storybookjs/storybook/pull/8000/files#diff-c4bcb815683c4c92762d0a868383a06cR38
Storybook was unable to detect filename for stories of kind "${kind}".
To fix it, add following to your jest.config.js:
transform: {
// should be above any other js transform like babel-jest
'^.+\\\\.stories\\\\.js$': '@storybook/addon-storyshots/injectFileName',
}
Which one is the correct advice?
Yes, I know that \\\\
is the JS equivalent of two backslashes output. However, I'm seeing all 4 backslashes on the console.
Hmm… I guess I answered my own question. I should be using two backslashes in my Jest config. The bug is in the console warning showing 4 backslashes.
Issue: #7697
What I did
– Migrated sample stories in
storyshots-core
to CSF–
Usedreverted in favor of #7878 (comment)displayName
in generated test names– Added detection of
fileName
fromreq
. It won't work withbabel-plugin-require-context-hook
until smrq/babel-plugin-require-context-hook#9 gets merged and released. Same forrequire-context.macro
storybookjs/require-context.macro#13– Added
@storybook/addon-storyshots/injectFileName
transform that addsexports.default.patameters.fileName
. Added a warning suggesting to include this transform whengetSnapshotFileName
is called butfileName
is falsy.