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 new react-docgen propTypes to info addon #1562

Merged
merged 12 commits into from
Aug 18, 2017
Merged

Add new react-docgen propTypes to info addon #1562

merged 12 commits into from
Aug 18, 2017

Conversation

danielduan
Copy link
Member

@danielduan danielduan commented Jul 31, 2017

What I did

This adds support for some new docgen propTypes such as union and includes some documentation on how to use info addon with react-docgen.

screen shot 2017-08-16 at 2 09 41 pm

Originally @jribeiro created #1505 to fix that and I'm making some minor changes to fix merge conflicts and redundant package.json requirements.

The original PR got automatically closed by GitHub when I tried to push to his branch so here's a new PR.

How to test

run cra example app

@danielduan
Copy link
Member Author

danielduan commented Aug 1, 2017

Hey @jribeiro, this seems to work for me for react elements without flow. if you check out this branch, the DocgenButton will display info whereas the TypedButton does not.

How were you able to make the flow type work with docgen?

@danielduan
Copy link
Member Author

Removing in progress label since I don't think flow types are possible with the way our babel plugin works.

@jribeiro
Copy link
Contributor

jribeiro commented Aug 7, 2017

Flow type is possible and should work just fine as implemented on the initial commit... you might need the flow-bin package though. I’ve just got back from holidays and been quite busy but should be able to take a look soon if you’re actually looking for contributions to the project

@danielduan
Copy link
Member Author

@jribeiro my bad, thanks for the update and heads up! I figured I haven't heard anything for a week so I might as well merge the parts I've tested.

We can give this another try. I'd love to support flow and add some documentation for it for Storybook. I think I might need to add a dependency or two to the example app and throw some flow tests in.

@danielduan
Copy link
Member Author

I opened up a separate PR to add Flow type removal to the default Babel config.

#1621

@jribeiro
Copy link
Contributor

jribeiro commented Aug 8, 2017

I added back the flowtype example and is working fine without additional modules as can be seen on this comment:
#1623 (comment)

And this screenshot:
screen shot 2017-08-08 at 20 52 52

Also added some more information to #1621 explaining why the babel-preset-flow is not needed.

Hope this helps.

Best
Joao

@jribeiro
Copy link
Contributor

jribeiro commented Aug 8, 2017

Ah! One last thing, with the imminent release of React Docgen version 3, you'll have problems with the hard coded dependency on react-docgen as seen on babel-plugin-react-docgen package https://github.com/storybooks/babel-plugin-react-docgen/blob/master/package.json#L31

This should be a peerDependency and not a dependency IMO but anyways version 3 brings
signature types - say () => * which this merge request already supports.

@danielduan
Copy link
Member Author

danielduan commented Aug 8, 2017

@jribeiro the babel-plugin-react-docgen needs a lot of work to make it work optimally. We basically check to see if it's a react component first, then pass it over to docgen. Docgen then checks if its a valid component again and does all the processing there.

We still miss some edge cases in the babel plugin in terms of detecting react components and I haven't tested it with docgen 3 yet. If you want to take a shot at it, feel free to open a PR.

I think peerDep * is something we can explore, ideally with a fallback of some sort so users don't have to manually install a version.

@jribeiro
Copy link
Contributor

jribeiro commented Aug 8, 2017

Cool. I'll try to have a look over the next couple of days. Cheers

@usulpro
Copy link
Member

usulpro commented Aug 16, 2017

@danielduan

I've seen this warning in cra-kitchen-sink:

warning.js:35 Warning: DocgenButton: type specification of prop `style` is invalid; the type checker
 function must return `null` or an `Error` but returned a function. You may have forgotten to pass an
 argument to the type checker creator (arrayOf, instanceOf, objectOf, oneOf, oneOfType, and shape all
 require an argument).

@danielduan danielduan changed the title Add react-docgen props to info addon Add new react-docgen propTypes to info addon Aug 16, 2017
@danielduan
Copy link
Member Author

Fixed the error and added a bunch more example docgen types.

@danielduan danielduan requested a review from usulpro August 16, 2017 18:59
@danielduan danielduan merged commit 2c289d7 into master Aug 18, 2017
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.

3 participants