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 kitchen sink to sample app #1224

Closed
wants to merge 17 commits into from
Closed

Added kitchen sink to sample app #1224

wants to merge 17 commits into from

Conversation

shilman
Copy link
Member

@shilman shilman commented Jun 8, 2017

Issue: #1220

What I did

Added notes, info, knobs, options to the test-cra and cra-storybook examples.

  • test-cra tries to simulate what a user would see installing these packages but based on file links
  • cra-storybook is uses lerna bootstrap to install the dependencies and is easier to develop on

What I found

info and knobs are apparently broken.

  • info shows an error in the preview window. this also causes our test suite to fail (yay?!)
  • knobs shows up but editing is broken (URL is not updated properly)

Update:

  • info works on all platforms?
  • knobs is fixed based on @ndelangen 's fix below

What I need

  • any feedback on existing scenarios
  • contributions for events, graphql addons
  • are the bugs i'm seeing actually bugs?

How to test

npm run bootstrap
cd examples cra-storybook
npm run storybook
cd ../test-cra
npm install
npm run storybook

@codecov
Copy link

codecov bot commented Jun 8, 2017

Codecov Report

Merging #1224 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1224   +/-   ##
=======================================
  Coverage   13.73%   13.73%           
=======================================
  Files         207      207           
  Lines        4638     4638           
  Branches      513      521    +8     
=======================================
  Hits          637      637           
+ Misses       3557     3547   -10     
- Partials      444      454   +10
Impacted Files Coverage Δ
app/react-native/src/bin/storybook-start.js 0% <ø> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <ø> (ø) ⬆️
app/react-native/src/bin/storybook.js 0% <ø> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
addons/info/src/components/PropVal.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/client_api.js 39.28% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 8.51% <0%> (ø) ⬆️
app/react/src/client/preview/render.js 0% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 33.33% <0%> (ø) ⬆️
... and 12 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 a81283f...b7b8080. Read the comment docs.

- add centered to test-cra
- add all to cra-storybook
Copy link
Member

@danielduan danielduan left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

import EventEmiter from 'eventemitter3';

import { storiesOf } from '@storybook/react';
import { action } from '@storybook/addon-actions';
import { linkTo } from '@storybook/addon-links';
import WithEvents from '@storybook/addon-events';
import { WithNotes } from '@storybook/addon-notes';
Copy link
Member

Choose a reason for hiding this comment

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

out of scope of this PR, but we should standardize whether we do import withAddon or import { withAddon }.

shilman pushed a commit that referenced this pull request Jun 9, 2017
shilman added a commit that referenced this pull request Jun 9, 2017
@alexandrebodin
Copy link
Contributor

#1238 Some cleaning I did on the propTypes :)

@shilman
Copy link
Member Author

shilman commented Jun 10, 2017

@tmeasday @ndelangen

Proposal based on what I've seen from both of you:

  1. rename cra-storybook => cra-kitchen-sink (to line up with react-native-vanilla, crna-vanilla)
  2. remove test-cra and port any interesting diffs over to cra-kitchen-sink
  3. create a cra-vanilla that's the equivalent to running getstorybook in a CRA app
  4. use bootstrap-style dependencies across the board rather than file: deps which are problematic

Happy to do all of this if you're good with it, except for (2) which I'd ask for help from @tmeasday

@tmeasday
Copy link
Member

I'm happy with this and happy to help. I'm not sure what the best naming is. I chose react-native-vanilla in the sense of "not CRNA" but I guess you took it to mean "simplest possible storybook usage".

I think long term we want a RN kitchen sink too, and there's no reason not to use the same app for that. So maybe we should rename that app react-native-vanilla-kitchen-sink or just react-native-kitchen-sink if it's clear that it isn't CRNA.

@shilman
Copy link
Member Author

shilman commented Jun 10, 2017

Ok, I understand vanilla better now.

How about:

  • x-getstorybook
  • x-kitchen-sink
  • x-whatever-we-want-to-demonstrate

@alexandrebodin
Copy link
Contributor

Sounds good

@shilman shilman closed this Jun 12, 2017
@ndelangen ndelangen deleted the 1220-kitchen-sink branch September 21, 2017 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants