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

CHANGE isToolshown logic so it hides toolbar if there's only (hidden) canvas #10599

Merged
merged 3 commits into from
Jun 11, 2020

Conversation

ndelangen
Copy link
Member

Issue: #10592

What I did

Fixed a bug where the toolbar might be shown with nothing in it.

@ndelangen ndelangen added this to the 6.0 milestone Apr 30, 2020
@ndelangen ndelangen self-assigned this Apr 30, 2020
@hipstersmoothie
Copy link
Contributor

Will this also make storybook navigate to the first non hidden tab?

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Not obvious to me how this corresponds to the original issue. Can you provide a test, or a comment on why this logic makes sense?

Comment on lines +142 to +143
const isToolshown =
!(viewMode === 'docs' && tabs.filter((t) => !t.hidden).length < 2) && options.isToolshown;
Copy link
Member Author

Choose a reason for hiding this comment

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

When the viewMode is docs, we only show tabs
When the number of tabs is 1, there's no point in showing them
When we don't show anything, the toolbar should be collapsed

AFAIK this was exactly the issue in #10592

This code makes it so when there's only 1 tab shown in docsmode the toolbar collapses

Copy link
Member

Choose a reason for hiding this comment

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

I didn't repro #10592 but @hipstersmoothie 's issue is that "nothing is rendered". AFAICT this change renders even less than what was there before, so I don't understand how this could solve that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can set up an example repo if neeed

Copy link
Member

Choose a reason for hiding this comment

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

Best is repro in official-storybook

Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen @shilman I've ran this locally and the only issue I see is that on initial load,
the MDX docs-only page that I select on SET_STORIES shows the toolbar like:

image

and after navigate another story with Canvas/Docs tab, and going back, it's not shown:

image

the tabs/reordering seems to work fine with beta.22 :D
now I'd like to know how to set the selectedTab in a similar way as selectedPanel hehe

@matheo
Copy link
Member

matheo commented May 5, 2020

@ndelangen I've confirmed that one of my issues (#10294)
was caused by moving the docs panel to the left with:

addons.setConfig({
  previewTabs: {
    'storybook/docs/panel': { index: -1 },
  },
});

@stale
Copy link

stale bot commented Jun 5, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added inactive and removed inactive labels Jun 5, 2020
@ndelangen
Copy link
Member Author

I'm just gonna close this if those doesn't fix any issues people are having.

@ndelangen ndelangen closed this Jun 8, 2020
@matheo matheo reopened this Jun 8, 2020
@matheo
Copy link
Member

matheo commented Jun 8, 2020

@ndelangen this fixes an empty toolbar for docsOnly pages:

image

the only missing detail is the initial-state, but this does PR the job

@ndelangen
Copy link
Member Author

@matheo would you have some time to make any changes to this PR you deem necessary?

@matheo
Copy link
Member

matheo commented Jun 10, 2020

@ndelangen I'm not quite familiar with React's state management to figure out the initial-value fix. For me this PR does an important part of the job tho. I'm +1 to merge this and avoid the empty toolbar div first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants