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

fix(486) - Only show props if there are props #488

Merged
merged 4 commits into from Jun 12, 2017
Merged

fix(486) - Only show props if there are props #488

merged 4 commits into from Jun 12, 2017

Conversation

SaraVieira
Copy link
Collaborator

Closes #486

@codecov-io
Copy link

codecov-io commented Jun 11, 2017

Codecov Report

Merging #488 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   94.35%   94.54%   +0.18%     
==========================================
  Files          96       96              
  Lines        1258     1283      +25     
  Branches      273      283      +10     
==========================================
+ Hits         1187     1213      +26     
+ Misses         68       67       -1     
  Partials        3        3
Impacted Files Coverage Δ
src/rsg-components/slots/UsageTabButton.js 100% <100%> (+33.33%) ⬆️
...ponents/TableOfContents/TableOfContentsRenderer.js 100% <0%> (ø) ⬆️
src/rsg-components/Usage/Usage.js 100% <0%> (ø) ⬆️
...omponents/ReactComponent/ReactComponentRenderer.js 100% <0%> (ø) ⬆️
src/rsg-components/Table/TableRenderer.js 100% <0%> (ø) ⬆️
src/rsg-components/Usage/UsageRenderer.js 100% <0%> (ø) ⬆️

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 07cf4d1...cb6576e. Read the comment docs.

@@ -74,6 +75,7 @@ export default class ReactComponent extends Component {
: <ExamplePlaceholder name={name} />
}
tabButtons={
showPropsButton &&
Copy link
Member

Choose a reason for hiding this comment

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

There may be other tabs, not just props. I’m thinking that this may be not that easy to fix ;-)

@SaraVieira
Copy link
Collaborator Author

SaraVieira commented Jun 11, 2017 via email

@sapegin
Copy link
Member

sapegin commented Jun 11, 2017

Doesn’t matter (there are none now) but the Slot component can render any number of items. So the props tab button should contain a check if there are props or methods and render null if there are none.

@@ -44,6 +44,7 @@ export default class ReactComponent extends Component {
const { component } = this.props;
const { name, slug, pathLine } = component;
const { description, examples = [], tags = {} } = component.props;
const showPropsButton = component.props.props || (component.props.methods || {}).length > 0;
Copy link
Member

Choose a reason for hiding this comment

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

Also this check won’t work:

  • !![] === true
  • ({}).length === undefined 

Copy link
Member

Choose a reason for hiding this comment

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

@SaraVieira
Copy link
Collaborator Author

Didn't know that about the slot but the props button receives the whole props including the methods and props of the component so I updated the code to only show or hide the methods component @sapegin

@@ -54,7 +54,7 @@
"readable-stream": {
"version": "2.2.11",
"resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.2.11.tgz",
"integrity": "sha512-h+8+r3MKEhkiVrwdKL8aWs1oc1VvBu33ueshOvS26RsZQ3Amhx/oO3TKe4lApSV9ueY6as8EAh7mtuFjdlhg9Q==",
"integrity": "sha1-B5azH412iAB/8Lk6gIjTSqF8D3I=",
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you have some weird version of npm. Why it changes all hashes to SHA1? ;-/

1. More consistent condition code
2. Return null insatead of undefined
3. Add PropTypes
4. Add tests
@sapegin sapegin merged commit 0d0ca5a into master Jun 12, 2017
@sapegin
Copy link
Member

sapegin commented Jun 12, 2017

I’ve made some tweaks (see my commits) and merged. Thanks!

@sapegin sapegin deleted the 486 branch June 12, 2017 12:10
@SaraVieira
Copy link
Collaborator Author

@sapegin Awesome !

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.

None yet

3 participants