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

Don’t render leftpanel stories tree if stories are empty #1664

Conversation

danielsneijers
Copy link
Contributor

Issue: Fixes #1588, Warnings about missing selectedKind and selectedStory on initial page render.

What I did

Stopped the LeftPanel from trying to render a Stories hierarchy tree when there are no stories to render.

Code in client init script of @storybook/react gets executed after LeftPanel gets mounted, so tweaking the code in the react client init script as suggested by @edoroshenko did not have any effect. Instead of making the propTypes in the Stories tree optional it seems more logical to not render it at all when there's no content to render.

How to test

@danielsneijers danielsneijers changed the title don’t render leftpanel stories tree if stories are empty Don’t render leftpanel stories tree if stories are empty Aug 15, 2017
@@ -46,7 +46,9 @@ class LeftPanel extends Component {
onChange={text => onStoryFilter(text)}
/>
<div style={scrollStyle}>
{storiesHierarchy ? <Stories {...pick(this.props, storyProps)} /> : null}
{storiesHierarchy && storiesHierarchy.map.size
Copy link
Member

Choose a reason for hiding this comment

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

It worth extracting this to a function for readability

@codecov
Copy link

codecov bot commented Aug 15, 2017

Codecov Report

Merging #1664 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1664      +/-   ##
==========================================
+ Coverage   21.11%   21.15%   +0.03%     
==========================================
  Files         247      247              
  Lines        5580     5579       -1     
  Branches      672      669       -3     
==========================================
+ Hits         1178     1180       +2     
- Misses       3896     3910      +14     
+ Partials      506      489      -17
Impacted Files Coverage Δ
...b/ui/src/modules/ui/components/left_panel/index.js 100% <100%> (ø) ⬆️
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 23.25% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
addons/info/src/components/PropTable.js 22.36% <0%> (ø) ⬆️
addons/info/src/components/markdown/code.js 24.13% <0%> (ø) ⬆️
addons/info/src/components/Props.js 37.2% <0%> (ø) ⬆️
lib/codemod/src/transforms/update-addon-info.js 53.65% <0%> (ø) ⬆️
addons/info/src/components/markdown/htags.js 30% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_routing.js 28.04% <0%> (ø) ⬆️
... and 21 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 d486e17...6b3cd3e. Read the comment docs.

@danielsneijers
Copy link
Contributor Author

By the way, concerning the codebeat issue: is that something I should fix or is it acceptable because it's a React component render function?

@igor-dv
Copy link
Member

igor-dv commented Aug 16, 2017

Usually we look into these issues, but it depends. @ndelangen, what do you think about this case ? I don't see how it can be improved =)

@ndelangen
Copy link
Member

Thank you for this PR @danielsneijers !

The tools like codebeat are helpers to stay alert.

I'm not aiming for 100% score in a list of arbitrary code-analytics tools. The point is to make reviews easier and keep us aware of issues we could solve when we touch code.

Great job on this PR!

@danielsneijers
Copy link
Contributor Author

Happy to help. Just to clarify, is any action needed from me (codebeat and circleci are still pending) or is one of the owners/members responsible for the updating and merging from this point?

@ndelangen
Copy link
Member

Nothing else is required to get this merged, your work on this PR is done!

Will merge it!

@ndelangen ndelangen merged commit d2be2c8 into storybookjs:master Aug 16, 2017
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.

3 participants