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

Add Storyshots - Closes #96 #101

Closed
wants to merge 1 commit into from
Closed

Add Storyshots - Closes #96 #101

wants to merge 1 commit into from

Conversation

osdevisnot
Copy link
Contributor

What does this PR addresses ?

  • Rename test directory to __test__ which aligns more with what Jest expects. The jest snapshot directory then is __test__/__snapshots__
  • Change storybook config to use require.context based on relative path (vs webpack resolved path) - storyshots currently errors out on webpack resolved path in context
  • Mark *.lock as binary files which skips diffs on yarn.lock file in PRs

What would this PR change ?

Tests BEFORE:
screen shot 2017-02-15 at 12 46 12 am
Tests AFTER:
screen shot 2017-02-15 at 12 46 25 am

Coverage BEFORE:
screen shot 2017-02-15 at 12 59 59 am
Coverage AFTER:
screen shot 2017-02-15 at 1 00 09 am

@codecov-io
Copy link

Codecov Report

Merging #101 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #101    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          39     76    +37     
  Lines         257    409   +152     
  Branches       51     51            
======================================
+ Hits          257    409   +152
Impacted Files Coverage Δ
test/componentsMock.js 100% <ø> (ø)
src/components/atoms/Block/index.stories.js 100% <ø> (ø)
src/components/organisms/Hero/index.stories.js 100% <ø> (ø)
src/components/atoms/LogoImage/index.stories.js 100% <ø> (ø)
src/components/atoms/Input/index.stories.js 100% <ø> (ø)
src/components/atoms/Tooltip/index.stories.js 100% <ø> (ø)
src/components/pages/SamplePage/index.stories.js 100% <ø> (ø)
src/components/pages/HomePage/index.stories.js 100% <ø> (ø)
...c/components/molecules/Blockquote/index.stories.js 100% <ø> (ø)
src/components/atoms/Badge/index.stories.js 100% <ø> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19ac506...afc16b9. Read the comment docs.

@diegohaz
Copy link
Owner

Thank you very much for your work, @abhishekisnot. Now we have a good view of what it would look like with storyshots.

The problem is everything we add to the boilerplate is one more thing for new developers to figure out. And, honestly, analyzing the snapshots, I don't think it's worth it. Most of them are just spans because we are mocking components in test/componentsMock.js to ensure isolated unit tests.

@diegohaz
Copy link
Owner

I didn't know about the yarn.lock trick though. Merging it is always really annoying 😆.

Can you put that in a separate PR?

@osdevisnot
Copy link
Contributor Author

Yeah, from value proposition, this makes sense. Thank you for considering the feature.

@osdevisnot
Copy link
Contributor Author

Just a sidenote, if we do not mock the component for tests, I start getting errors on Jest tests as:

TypeError: require.context is not a function

It seems, Jest does not/won't support dynamic requires as mentioned here:
facebook/create-react-app#517
jestjs/jest#2298

@diegohaz
Copy link
Owner

Yeah, Jest doesn't support that. And that's another reason to mock components.

I agree with Dan. require.context is kinda obscure, but it really improves developer experience, especially in our case (can use Atomic Design structure without pain).

Anyway, thank you, @abhishekisnot. I'll close this PR, but I think it's still valuable as a reference for one wanting to add this feature to their projects.

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

Successfully merging this pull request may close these issues.

3 participants