-
-
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
Add addon jest #2295
Add addon jest #2295
Conversation
@shilman is a new addon considered a "small feature"? |
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.
yarn.lock should be removed as well: we use yarn workspaces feature, so all the dependencies are installed by root yarn bootstrap
command
addons/jest/.eslintignore
Outdated
@@ -0,0 +1,2 @@ | |||
node_modules | |||
dist |
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.
Those should be covered by root .eslintignore file
addons/jest/.eslintrc.js
Outdated
}, | ||
}, | ||
}; | ||
|
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.
Let's try to use our root eslint config for this package, and override only the things that really need it, if any
require('../List.story'); | ||
} | ||
|
||
configure(loadStories, module); |
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.
Let's move the stories to example/cra-kitchen-sink
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.
agreed
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.
I am on it.
addons/jest/package.json
Outdated
}, | ||
"scripts": { | ||
"prebuild": "npm run clear", | ||
"build": "cross-env NODE_ENV=production babel -d ./dist ./src", |
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.
We need a prepare
script instead (see other addons). All the babel-related stuff can be removed from package as we use top-level babel dependencies and setup for transpiling everything
addons/jest/package.json
Outdated
"jest": "^21.2.1", | ||
"react": "^16.1.0", | ||
"react-dom": "^16.1.0", | ||
"style-loader": "^0.19.0" | ||
}, | ||
"peerDependencies": { | ||
"@storybook/addons": "^3.2.14", | ||
"prop-types": "^15.6.0", |
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.
It should be a simple dependency, not a peer one
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.
I actually meant the prop-types
package. It's OK for an end user not to have it
Codecov Report
@@ Coverage Diff @@
## master #2295 +/- ##
=========================================
- Coverage 21.23% 20.14% -1.1%
=========================================
Files 283 293 +10
Lines 6155 6489 +334
Branches 727 755 +28
=========================================
Hits 1307 1307
- Misses 4298 4595 +297
- Partials 550 587 +37
Continue to review full report at Codecov.
|
Ready to merge I think? @Hypnosphi ? |
Please move |
cb3d4d6
to
d185d1a
Compare
Actually, I still don't get what's happening here =( Why those green and red bars have different length? Why storybook is a place to show test results in a fancy way? Why would anybody want to make empty stories? |
addons/jest/README.md
Outdated
``` | ||
|
||
**Known issue**: if you use a deploy script using for example `gh-pages`, be sure to not put | ||
the `test` script that write the result in part of the script process (in `predeploy` for example). |
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.
Why?
addons/jest/README.md
Outdated
|
||
Brings Jest results in storybook. | ||
|
||
[![Storybook Jest Addon Demo](@storybook/addon-jest.gif)](https://@storybook/addon-jest-example.herokuapp.com/) |
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.
The link is broken
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.
What's up with the SimpleList
CI failures? Looks like some files didn't get checked in?
Aside from that, this looks great. @Hypnosphi this is a new feature so technically it should probably go into 3.3, but it's isolated and shouldn't break anything so I'm also OK with it to go into a patch.
Very excited to get this into peoples' hands in one way or another and look forward to getting a cleaned up version merged soon after the changes have been addressed.
Actually isolation is an issue here. Once someone adds this addon for some of the stories, nobody can |
addons/jest/README.md
Outdated
} | ||
``` | ||
|
||
Add it the result file to `.gitignore`: |
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.
@renaudtertrais @ndelangen why do we even have this recommendation? Checking this file into VCS makes a lot of sense to me, given that it's used in an import
. What's the downside?
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.
Well, it will likely have tons of merge-conflicts over time?
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.
Just like lockfiles and test snapshots. It's easy to regenerate them, so this shouldn't be a problem
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.
true
I will change the text and make it so users are aware of the trade-offs.
@shilman, @Hypnosphi I think I've resolved all issues? Can you accept? |
@shilman The thing you mentioned has been resolved, I hope you're OK with me merging this. |
Issue:
What I did
Add addon-jest to monorepo
How to test
In storybook example