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

Added a vanilla React Native example app. #1202

Merged
merged 5 commits into from
Jun 7, 2017
Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jun 6, 2017

Issue: We need an example app for RN! #1012

What I did

cd examples
react-native init ReactNativeVanilla
cd ReactNativeVanilla
# --> checked it ran normally

getstorybook
# --> checked it ran -- actually there was an issue #1200 I worked around

# --> updated package.json w/ file:// ref to @storybook/react-native
npm install
# --> checked it ran -- I had to revert to RN 0.44.1 because of https://github.com/facebook/react-native/issues/14209]

# --> updated package.json w/ file:// ref to @storybook/addon-storyshots
npm install
# --> added `__tests__/storyshots.js` test file
npm test # <- ensured it worked as expected.

# --> updated package.json w/ file:// ref to @storybook/addon-options
# --> Added code to use options in `storybook/addons.js` and `storybook/index.ios.js` 
#    (note had to work around https://github.com/storybooks/storybook/issues/1192 with a `setTimeout`)
npm run storybook / react-native run-ios # <-- to check the options worked.

How to test

  1. Checkout, npm install.
  2. Run the storybook via (a) npm run storybook + react-native run-ios. Check the stories are displayed and the name option works.
  3. Run npm test, ensure 3 snapshots are recorded.

@tmeasday
Copy link
Member Author

tmeasday commented Jun 6, 2017

Thanks CodeFactor! I tried to work around https://www.codefactor.io/repository/github/storybooks/storybook/pull/1202 but could not :)

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Please give me a chance to review before merging 😸

@tmeasday
Copy link
Member Author

tmeasday commented Jun 6, 2017

@shilman see in this description why I didn't just import a common file: #1201

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@tmeasday you need to add this to lerna.json to ignore for both publish and bootstrap. We don't want to publish it. For bootstrap, it fails if I run a clean bootstrap from the root. But if I go to the RN app directory and npm install it works.

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

Should we keep all folder-names lowercase and dash separated ?

@tmeasday
Copy link
Member Author

tmeasday commented Jun 6, 2017

React Native wouldn't let me pass such a name into it. I'm scared to change it after the fact ;) [I'll try]

@tmeasday
Copy link
Member Author

tmeasday commented Jun 6, 2017

Ok, made the changes requested

@codecov
Copy link

codecov bot commented Jun 6, 2017

Codecov Report

Merging #1202 into master will decrease coverage by 0.4%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1202      +/-   ##
==========================================
- Coverage   13.72%   13.32%   -0.41%     
==========================================
  Files         207      199       -8     
  Lines        4611     4585      -26     
  Branches      520      618      +98     
==========================================
- Hits          633      611      -22     
+ Misses       3523     3444      -79     
- Partials      455      530      +75
Impacted Files Coverage Δ
addons/knobs/src/components/types/Date/index.js 54.54% <0%> (-18.19%) ⬇️
lib/ui/src/modules/ui/components/layout/index.js 73.58% <0%> (-4.99%) ⬇️
lib/ui/src/modules/ui/components/shortcuts_help.js 42.85% <0%> (-2.6%) ⬇️
addons/knobs/src/components/types/Array.js 77.77% <0%> (-2.23%) ⬇️
addons/knobs/src/components/types/Boolean.js 9.52% <0%> (-2.11%) ⬇️
lib/ui/src/modules/ui/components/search_box.js 14.81% <0%> (-1.86%) ⬇️
.../ui/src/modules/ui/components/left_panel/header.js 25.92% <0%> (-1.67%) ⬇️
addons/knobs/src/components/types/Color.js 6.34% <0%> (-1.66%) ⬇️
addons/knobs/src/components/PropForm.js 7.14% <0%> (-1.56%) ⬇️
addons/knobs/src/components/types/Object.js 4.81% <0%> (-1%) ⬇️
... and 94 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 de765c6...4a5dfbd. Read the comment docs.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

thanks!!

@shilman
Copy link
Member

shilman commented Jun 6, 2017

@tmeasday build is failing in minor ways

@tmeasday tmeasday force-pushed the 1012-react-native-example branch 2 times, most recently from d8436a8 to 481f2c9 Compare June 6, 2017 12:00
For #1012. Includes storyshots and the use of addon-options
Although RN uses Jest for testing, it uses a Jest preset which messes with the environment. AFIACT it's not possible to selectively apply the environment to different tests so we just separate them.
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@shilman shilman merged commit a6e1af3 into master Jun 7, 2017
@shilman shilman deleted the 1012-react-native-example branch June 7, 2017 12:56
@ndelangen ndelangen added this to the v3.0.2 milestone Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks react-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants