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 CRNA app with getstorybook #1219

Closed
wants to merge 11 commits into from
Closed

Conversation

shilman
Copy link
Member

@shilman shilman commented Jun 8, 2017

Issue: #1218

What I did

Equivalent of:

yarn create react-native-app crna-getstorybook
cd crna-getstorybook
getstorybook

Plus added a few /* eslint-disable */ for files generated by create-react-native-app which don't follow our coding conventions and added snapshot testing.

I did not hook up storybook with the app, per
https://github.com/storybooks/storybook/tree/master/app/react-native#create-react-native-app-crna

How to test

Automated test:

yarn bootstrap
cd examples/crna-getstorybook
yarn test

Manual test:

yarn bootstrap
https://github.com/storybooks/storybook/pull/1219
yarn storybook
yarn ios

Equivalent of:
```
yarn create react-native-app crna-vanilla
cd crna-vanilla
getstorybook
```

I did not hook up storybook with the app, per
https://github.com/storybooks/storybook/tree/master/app/react-native#cre
ate-react-native-app-crna
@ndelangen ndelangen added this to the v3.1.0 milestone Jun 8, 2017
@codecov
Copy link

codecov bot commented Jun 8, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1219      +/-   ##
==========================================
- Coverage   13.75%   13.61%   -0.14%     
==========================================
  Files         207      207              
  Lines        4632     4678      +46     
  Branches      510      630     +120     
==========================================
  Hits          637      637              
+ Misses       3556     3508      -48     
- Partials      439      533      +94
Impacted Files Coverage Δ
app/react-native/src/bin/storybook-build.js 0% <ø> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <ø> (ø) ⬆️
app/react-native/src/bin/storybook.js 0% <ø> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 49.42% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 33.33% <0%> (ø) ⬆️
app/react/src/client/preview/render.js 0% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
app/react/src/server/build.js 0% <0%> (ø) ⬆️
addons/events/src/components/Panel.js 0% <0%> (ø) ⬆️
... and 23 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 b32cae6...88dd5eb. Read the comment docs.

@tmeasday
Copy link
Member

tmeasday commented Jun 9, 2017

Is this ready for review?

@shilman
Copy link
Member Author

shilman commented Jun 9, 2017

@tmeasday please see my comments in what i need above

@tmeasday
Copy link
Member

tmeasday commented Jun 10, 2017

@shilman, my answer to the above was: it doesn't look like it'll be possible to hijack the packager in the same way for CRNA. Instead of the vanilla packager, a CRNA app ends up using something deep in the bowels of xdl (Expo's lib) to package the app, AFAICT. It's all programatic so it doesn't seem like it'd be possible to pass configuration through and change the projectRoot that it packages (that's what we do in the vanilla RN case).

@shilman
Copy link
Member Author

shilman commented Jun 10, 2017

@tmeasday any thoughts on the best way to make this testable?

@tmeasday
Copy link
Member

We can use jest to test it with storyshots, right? If you want to test the actual native app connection works, I think that's pretty complicated :/

@shilman shilman changed the title Added vanilla CRNA app with storybook Added CRNA app with getstorybook Jun 10, 2017
@alexandrebodin
Copy link
Contributor

Hey.

I think we need to decide for a n example folder structure. Here is a proposal to apply here and in #1224

Current state

examples
|-- cra-storybook
|-- react-native-vanilla
|-- test-cra
|-- crna-getstorybook // This PR

Goals

  • Have clear getstarted examples for both react and react-native and any other lib we plan on supporting
  • Have examples for addons usage
  • Have a good project for us to develop / experiment on and test addons and new features.
  • Have a live storybook for sotrybook source code documentation

Proposal

examples
|-- react
	|-- getstarted // would be cra + getstorybook
	|-- advanced // vanilla + addons
|-- react-native
	|-- getstart // crna + getstorybook
	|-- advanced // same
|-- storybook // testing our components / development workplace / experimenting => Could be live on the website to serve as a live demo.

I tried to synthesize some of the discussion we all had. Sorry If I missed sth :)

@shilman
Copy link
Member Author

shilman commented Jun 11, 2017

@alexandrebodin my proposal was here: #1224 (comment)

I prefer a flat file structure both because it's simpler with Lerna and also simpler for a user to scan all of the examples in one place.

@alexandrebodin
Copy link
Contributor

Aren't you afraid there will be too many flat projects if we start supporting angular cycle vue etc ? For now we can go with your idea of flat strucutre and wait for future evolution.

I don't think it's really clear for a user to see cra-getstorybook imho

i would at least expect react-getstarted (rather thant react-getstorybook as you suggested I think)

That's only my opinion of course :)

@shilman
Copy link
Member Author

shilman commented Jun 11, 2017

@alexandrebodin You're right that when we start to support multiple frameworks we should divide it up. Currently we just support two though, so let's keep it simple to start and refactor once we have more frameworks.

I'm fine with renaming x-getstorybook to x-getstarted and will rename accordingly in both PRs

@shilman
Copy link
Member Author

shilman commented Jun 12, 2017

Merging this into #1224 and closing

@Hypnosphi Hypnosphi deleted the 1218-crna-example-app branch October 11, 2017 22:31
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.

4 participants