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

UI: Display menu icon on the toolbar when the sidebar is collapsed #15369

Merged
merged 12 commits into from
Jun 30, 2021

Conversation

apapadopoulou
Copy link
Member

@apapadopoulou apapadopoulou commented Jun 25, 2021

Issue: #13799

What I did

I added a new ToolbarMenu type in the Menu.tsx file and created a new menu tool, then added the tool in the left side of the toolbar. I also added a new story in the Menu.stories.tsx file and fixed two stories that had some errors. However, there are some problems, which I list below and this is the reason why I created a draft pull request initially.

  • The display of the icon is only successful whenever you load a preview story with the sidebar collapsed. Then, using the shortcut S you can make the sidebar appear again and the icon disappears and appears again when you use the shortcut. Navigation from the menu is not successful. To make the menu icon appear, I used the state.layout.showNav property, but maybe this is not the right way and this is the cause of the above problems. However, I noticed that the above bugs only occurred in my instance of storybook, whereas the icon is displayed as intended in the deploy preview.
  • I also planned to add a new preview story where the icon is visible, however there were some changes in the preview.tsx file in the components module and I think that now all of the preview stories should be modified.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
    I added new menu stories, which are visible in the official-example app.
  • Does this need an update to the documentation?
    I am not sure but I can update the documentation if needed.

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jun 25, 2021

Nx Cloud Report

CI ran the following commands for commit 730ff9f. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@apapadopoulou apapadopoulou marked this pull request as ready for review June 25, 2021 18:02
@apapadopoulou apapadopoulou requested a review from domyen June 26, 2021 14:13
@shilman
Copy link
Member

shilman commented Jun 28, 2021

This is awesome @apapadopoulou. I made an animated GIF for people to check out more easily:

sidebar-toggle

Copy link
Contributor

@darleendenno darleendenno left a comment

Choose a reason for hiding this comment

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

Hey @apapadopoulou! Thank you for taking the time to improve Storybook! I really like this change -- it makes the sidebar much easier to find when collapsed 😄 I have a couple of non-blocking suggestions to make your code even better and more accessible. Great work! ❤️

lib/ui/src/components/sidebar/Menu.tsx Outdated Show resolved Hide resolved
lib/ui/src/components/preview/tools/menu.tsx Outdated Show resolved Hide resolved
@MichaelArestad
Copy link
Contributor

MichaelArestad commented Jun 28, 2021

This is pretty cool. I like that you're adding a way to prevent folks from losing the sidebar.

One thing that struck me as I was trying this out was that I expected the icon to reopen the sidebar instead of opening the menu. This would save users a step if they are trying to reopen the sidebar. What do you think about trying that route?

@apapadopoulou
Copy link
Member Author

This is pretty cool. I like that you're adding a way to prevent folks from losing the sidebar.

One thing that struck me as I was trying this out was that I expected the icon to reopen the sidebar instead of opening the menu. This would save users a step if they are trying to reopen the sidebar. What do you think about trying that route?

Thank you for the feedback! I made it that way to help the user quickly access the shortcuts menu, as pointed out by @domyen on issue #13799. I could change it to show the sidebar again after clicking, but then the access to the menu would not be that instant.

@apapadopoulou
Copy link
Member Author

Hey @apapadopoulou! Thank you for taking the time to improve Storybook! I really like this change -- it makes the sidebar much easier to find when collapsed 😄 I have a couple of non-blocking suggestions to make your code even better and more accessible. Great work! ❤️

Thank you!! I will look into the suggestions one by one.

@domyen
Copy link
Member

domyen commented Jun 29, 2021

Nice PR!

re:sidebar toggle, The intent of revealing the icon when the sidebar was collapsed was to give folks an easy way to show the sidebar again. The default state of Storybook is to show the sidebar so we want to provide a one click way to get back to that state.

@apapadopoulou
Copy link
Member Author

Nice PR!

re:sidebar toggle, The intent of revealing the icon when the sidebar was collapsed was to give folks an easy way to show the sidebar again. The default state of Storybook is to show the sidebar so we want to provide a one click way to get back to that state.

Okay, should I change it to display the sidebar when clicked?

@domyen
Copy link
Member

domyen commented Jun 29, 2021

Okay, should I change it to display the sidebar when clicked?

Yes please 🙇‍♂️

Copy link
Contributor

@darleendenno darleendenno left a comment

Choose a reason for hiding this comment

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

This is looking so good! A few alterations and this should be good! 🎉

lib/ui/src/components/preview/tools/menu.tsx Outdated Show resolved Hide resolved
lib/ui/src/components/preview/tools/menu.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@darleendenno darleendenno left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉 thank you! 🎉 🎉 🎉

this is a wonderful experience:
toggle

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.

This is great @apapadopoulou -- love how it turned out! 😻

Copy link
Contributor

@MichaelArestad MichaelArestad left a comment

Choose a reason for hiding this comment

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

Nice work!

@shilman shilman merged commit c74836d into storybookjs:next Jun 30, 2021
@domyen
Copy link
Member

domyen commented Jun 30, 2021

Thanks @apapadopoulou! 👏

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