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

More detailed props table #1485

Merged
merged 24 commits into from
Sep 6, 2017

Conversation

billyvg
Copy link
Contributor

@billyvg billyvg commented Jul 18, 2017

Issue:
PropTypes for shape, oneOf, and arrayOf could be more specific. There's lots of useful data missing for these types. Since this is a WIP, there's probably some better work to be done for other prop types as well.

Ex:

image

What I did

Changed PropTable to be more detailed by inspecting__docgenInfo and recursively formatting certain prop types.

How to test

Open a component in storybook that has these proptypes: oneOf, shape, arrayOf

@codecov
Copy link

codecov bot commented Jul 19, 2017

Codecov Report

Merging #1485 into release/3.3 will decrease coverage by 0.4%.
The diff coverage is 12.09%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #1485      +/-   ##
===============================================
- Coverage        23.05%   22.65%   -0.41%     
===============================================
  Files              248      264      +16     
  Lines             5703     5906     +203     
  Branches           673      690      +17     
===============================================
+ Hits              1315     1338      +23     
- Misses            3901     4069     +168     
- Partials           487      499      +12
Impacted Files Coverage Δ
addons/info/src/components/PropTable.js 22% <0%> (+0.99%) ⬆️
lib/components/src/index.js 0% <0%> (ø) ⬆️
lib/components/src/highlight_button.js 0% <0%> (ø)
addons/info/src/components/types/Object.js 0% <0%> (ø)
lib/components/src/table/table.js 0% <0%> (ø)
lib/components/src/table/cell.js 0% <0%> (ø)
addons/info/src/components/types/Enum.js 0% <0%> (ø)
addons/info/src/components/types/PrettyPropType.js 10.41% <14.28%> (ø)
addons/info/src/components/types/OneOfType.js 11.11% <14.28%> (ø)
addons/info/src/components/types/ArrayOf.js 15.38% <22.22%> (ø)
... and 52 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 40c652d...f2a9b66. Read the comment docs.

@shilman
Copy link
Member

shilman commented Jul 22, 2017

@billyvg awesome--thanks so much for putting this together? Is there any chance you can add this to the examples/cra-kitchen-sink example as part of this PR? We're starting to make sure that all PRs have good examples there.

To get the example running on your machine:

yarn && yarn bootstrap
cd examples/cra-kitchen-sink
yarn storybook

Many thanks again and please let me know if you have any questions!

@ndelangen ndelangen changed the title [addon-info] [WIP] More detailed props table [WIP] More detailed props table Jul 28, 2017
@ndelangen
Copy link
Member

Added the do-not-merge tag because it's a WIP.
Rebased unto master.

@billyvg Please ask us anything if you need help! Thank you for this PR!

@ndelangen
Copy link
Member

@billyvg Are you still working on this?

@ndelangen
Copy link
Member

related: #1147

@billyvg
Copy link
Contributor Author

billyvg commented Aug 18, 2017

I am! I'm having trouble deciding how to design nested arrayOf/shape types. I have another branch that flattens the props like

property proptype
shape shape
shape.nested array
shape.nested[] string
shape.foo shape
shape.foo.bar string

Any suggestions?

@ndelangen
Copy link
Member

Good to hear, looking forward to this PR completing! 🎈

Here's an out of the box suggestion: https://github.com/xyc/react-inspector
Could that be a nice UI for this?

Otherwise I'm liking the original screen quite a lot actually!

* more details for arrayOf, objectOf, shape, oneOf, oneOfType
* refactored into cleaner code + many smaller components
@billyvg
Copy link
Contributor Author

billyvg commented Aug 23, 2017

@ndelangen Cleaned up the code a bunch as well as improve the formatting:

https://cl.ly/151I0X3Q0O34/Screen%20Recording%202017-08-23%20at%2003.40%20PM.gif

@ndelangen ndelangen changed the base branch from master to release/3.3 August 24, 2017 08:11
@ndelangen
Copy link
Member

@billyvg That looks fantastic!

I rebased this to release 3.3 branch. I hope this doesn't influence you in a bad way.

@ndelangen
Copy link
Member

This PR is going to be awesome! The move to lib/components and glamorous is fantastic @billyvg

Just as a FYI: @marchdoe ;)

import React from 'react';
import glamorous from 'glamorous';

const Button = props => <span role="button" tabIndex={-1} {...props} />;
Copy link
Member

Choose a reason for hiding this comment

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

Why span not button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not want to have to reset/restyle a button

Copy link
Member

Choose a reason for hiding this comment

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

Native buttons have some advantages like automatically triggering click event on Enter. I believe it’s worth a bit of extra styling

import { TypeInfo } from './proptypes';

const ObjectOf = ({ propType }) =>
<span>
Copy link
Member

Choose a reason for hiding this comment

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

@storybookjs storybookjs deleted a comment from billyvg Aug 30, 2017
Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

Please use native buttons

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

LGTM

@ndelangen
Copy link
Member

ndelangen commented Sep 6, 2017

@billyvg Is this still a WIP?

If not, remove the [WIP] in the header and we can do a final review and get this merged!
I've just tested this, and added a few usages in cra-kitchen-sink, all green here.

@billyvg billyvg changed the title [WIP] More detailed props table More detailed props table Sep 6, 2017
@billyvg
Copy link
Contributor Author

billyvg commented Sep 6, 2017

@ndelangen I think it's no longer a WIP, I've removed the tag.

@ndelangen ndelangen merged commit 54f01b0 into storybookjs:release/3.3 Sep 6, 2017
@ndelangen
Copy link
Member

ndelangen commented Sep 6, 2017

Congrats on your first merged PR @billyvg ! 🎉

I hope more will follow!

@billyvg billyvg deleted the prop-table-detailed branch September 6, 2017 13:22
@billyvg billyvg restored the prop-table-detailed branch December 9, 2017 00:09
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