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

[Feature Request] Add option to hide toolbars for doc pages #22081

Open
noranda opened this issue Apr 13, 2023 · 25 comments · May be fixed by #29516
Open

[Feature Request] Add option to hide toolbars for doc pages #22081

noranda opened this issue Apr 13, 2023 · 25 comments · May be fixed by #29516

Comments

@noranda
Copy link
Contributor

noranda commented Apr 13, 2023

Is your feature request related to a problem? Please describe

It's not currently possible to show toolbars for stories, but hide them for doc pages (see thread). I have unattached doc pages with no stories as well as doc pages with stories.

Describe the solution you'd like

I'd like to be able to hide the toolbar for unattached doc pages with no stories, where the toolbar options don't apply.

Describe alternatives you've considered

I could look into hiding the individual toolbar options (I believe SB 6.5 did this), but I'd like to just be able to hide the toolbar.

Are you able to assist to bring the feature to reality?

yes, I can

Additional context

No response

@shilman
Copy link
Member

shilman commented Apr 14, 2023

@noranda Great point!! This is the line of code that says show the toolbar no matter what the viewMode is:

https://github.com/storybookjs/storybook/blob/next/code/addons/toolbars/src/manager.tsx#L11

Here's another addon that only shows the toolbar for stories and never for docs:

https://github.com/storybookjs/storybook/blob/next/code/addons/measure/src/manager.tsx#L12

I'd be open to hide toolbars on unattached docs pages that don't render any stories. I'm too big a fan of making that behavior user configurable, since it will just lead to more documentation/maintenance. I'm not sure whether we have enough info inside that function to be smart about it for unattached docs. @tmeasday any thoughts?

@tmeasday
Copy link
Member

tmeasday commented Apr 14, 2023

I guess if the file has no stories imports @shilman? Won't work for legacy id based rendering but that's probably ok.

We do want to move the toolbar onto the canvas block though. Not sure how soon that'll happen however as it's a bit tricky.

@shilman
Copy link
Member

shilman commented Apr 14, 2023

@tmeasday I guess to get more specific:

https://github.com/storybookjs/storybook/blob/next/code/ui/manager/src/components/preview/toolbar.tsx#L230-L236

This is the calling code for that match function. If the callee (in this case the toolbars addon) has access to { storyId, refId, viewMode, location, path } is that enough for it to determine whether the file has no stories imports? Or would we need to compute that elsewhere to give addons the extra information they need to make that decision?

@jatinparmar96
Copy link

I created a PR for this issue, I want to know if we should, completely hide the toolbar like what happens when you disable the toolbar or just hide the tools? Also, what would be a good place to add the setting for toggling this functionality? If anyone could guide me, that would be helpful!

@JReinhold
Copy link
Contributor

@tmeasday I guess to get more specific:

next/code/ui/manager/src/components/preview/toolbar.tsx#L230-L236

This is the calling code for that match function. If the callee (in this case the toolbars addon) has access to { storyId, refId, viewMode, location, path } is that enough for it to determine whether the file has no stories imports? Or would we need to compute that elsewhere to give addons the extra information they need to make that decision?

@shilman

I can't imagine that would be enough to determine if there are stories in the docs or not.
I don't think this is about attached vs unattached docs though, but more about docs having stories or not (they can be attached without having stories).

We can't remove the toolbar completely, as it contains the fullscreen tool, and I would expect it to get more native tools later.

Are we certain that all addon tools can safely be removed if there are no stories? because we know they can only affect stories, and not the docs content?
I'm trying to dig into if we should always remove tools in docs without stories, or if we should instead make it possible for individual tools to determine if they should remove themselves if there are no stories, using the match function.

@shilman
Copy link
Member

shilman commented Apr 27, 2023

@JReinhold I don't know whether it's 100% safe to remove the addon-toolbars (or any addon that adds itself to the toolbar) if there are no stories on the docs page.

I guess, given that, the safest thing to do is give users the option of hiding them by configuration. Or to hide the entire toolbar.

@JReinhold
Copy link
Contributor

I guess, given that, the safest thing to do is give users the option of hiding them by configuration. Or to hide the entire toolbar.

I share your concern about the API though, I think it would make more sense to give addon authors the ability to more fine-grainily hide the tools, they should know if it makes sense or not to have them without stories.

@noranda
Copy link
Contributor Author

noranda commented Apr 27, 2023

@shilman @JReinhold If it helps, for my specific use case I'd like to be able to hide the Canvas and any addon tabs from the toolbar. That would just leave the full screen button, which I could live with or without. So either an option to hide the toolbar for a given doc page, or options to hide the Canvas and all addon tabs would work for me. I'm on Storybook 7.

@JReinhold
Copy link
Contributor

Do you want to do it for all your docs pages, or just some of them? If the latter, what makes them special from the rest?

@noranda
Copy link
Contributor Author

noranda commented Apr 27, 2023

Do you want to do it for all your docs pages, or just some of them? If the latter, what makes them special from the rest?

I'd like the option for some doc pages, but not all. For example, I have:

  1. A Welcome unattached doc page that doesn't need a toolbar nor any addons. I could live with or without the full screen button here.

Screenshot 2023-04-27 at 10 16 57 AM

  1. A Theming unattached doc page that doesn't need a Canvas or Playroom addon tab, but does need my theme switcher in the toolbar so I can view color palettes by theme.

Screenshot 2023-04-27 at 10 12 32 AM

  1. A component doc page that doesn't need the Canvas tab (each story has its own Canvas tab, so not needed in the Doc page), but does need Playroom addon tab as well as the theme switcher.

Screenshot 2023-04-27 at 10 12 48 AM

@tmeasday
Copy link
Member

Thanks for stepping in for my lack of response @JReinhold -- I think you are spot on.

We can't remove the toolbar completely, as it contains the fullscreen tool, and I would expect it to get more native tools later.

My understanding is the plan is to move the fullscreen button and any future native tools to more of a "button/menu" UI on the top RHS (of the docs page) and drop the toolbar entirely, then to put addon toolbars on top of the story in a toolbar on the Canvas block. (credit @MichaelArestad):

image

My opinion is we'd want to be pretty cautious making changes to the "defunct" docs-level toolbar that requires addon authors to do anything, given that we will likely require changes to support the above later (maybe 8.0?).

Adding a tag or something that a user can set to do something like @noranda suggested might make sense, as long as we were sure to tell them that likely it's a bandaid for a bigger change coming later.

@noranda
Copy link
Contributor Author

noranda commented May 3, 2023

@tmeasday Adding a config option to turn off toolbar options would be great for me with the understanding that the toolbar may undergo significant changes in an upcoming update. Is this something that can be added in the short term?

@tmeasday
Copy link
Member

tmeasday commented May 4, 2023

@JReinhold @shilman do y'all think it would make sense for users to be able to write a function that looks something like:

shouldShowToolbar(toolbarName: string, entry: IndexEntry): boolean

Then they could look at entry.type and entry.tags (or the title/name if they prefer) and the toolbar name to choose whether to hide it or not?

@Sidnioulz
Copy link
Contributor

@shilman @tmeasday Am I correct in thinking this new option should go in preview.js similarly to storySort?

From within the addon's match function, it seems to me we can't call hooks to retrieve any option or param. To get the option, assuming it's in preview.js, I'm thinking of using the same logic as code/lib/core-server/src/utils/StoryIndexGenerator.ts to retrieve the option from users.

If that's the way forward, I'll factorise this code into a shared util. I don't know, though, if there's a preferred way to get the CLI options in that context. I'm assuming that's known and available, and I'm sure I can find a way to identify it, but if there's a preferred method, feel free to point me in that direction :)

@tmeasday
Copy link
Member

tmeasday commented Aug 8, 2023

Am I correct in thinking this new option should go in preview.js similarly to storySort?

I think given the toolbar is rendered by the manager, the option would probably want to live in manager.js, and be configured like:

addons.setConfig({
  toolbar: {
    shouldShow(..) // not sure about naming    
  },
});

TBH I'm not 100% confident about how the manager configuration works but I'm looking at some sidebar configurations, and it seems they are just read like this:

const { sidebar = {} } = provider.getConfig();
const { showRoots, collapsedRoots = [], renderLabel } = sidebar;

We could probably do something similar with reading toolbar options here-ish:

const toolsFromConfig = useMemo(() => getTools(getElements), [getElements]);
const toolsExtraFromConfig = useMemo(() => getToolsExtra(getElements), [getElements]);

@Sidnioulz
Copy link
Contributor

Thanks Tom, I'll give it a try with that.

@Sidnioulz
Copy link
Contributor

Sidnioulz commented Aug 9, 2023

The good news is I managed to make it work. The bad news is that hiding the toolbar itself isn't enough, I should hide the space reserved within it by preview.

image

I think it would make more sense for the function to impact state.layout.showToolbar, which is already widely used in the code, than to add extra showing/hiding logic in the toolbar specifically. It'd avoid side effects like that one.

WDYT?

@tmeasday
Copy link
Member

tmeasday commented Aug 9, 2023

I guess changing the state on every route change might be a bit difficult/brittle. Did you have an idea of how you might do that @Sidnioulz?

@Sidnioulz
Copy link
Contributor

Hm, you're right. Updating the UI when I change the value would be easy, but knowing when to call my function again wouldn't.

I'll stick to the original plan then, but I'll host the logic in preview instead of the toolbar itself.

@Sidnioulz
Copy link
Contributor

Sidnioulz commented Aug 28, 2023

An update following a brainstorming session with Norbert. We found a way to get rid of the addons dependency and to extend this showToolbar function logic in a way that's testable and a bit more future-proof. Thanks Norbert for your help on this, I learnt a lot in the process :)

Here's what we came up with:

  • Instead of using addons, we store the user-defined customisation function inside the layout module of the manager, under a new key e.g. layoutCustomisations
  • Users continue to set functions in their instance's manager.ts file
  • The layout module, on init, reads manager.ts to set the functions in its state
  • As the functions are now stored in manager state, we can override them in FakeProvider when writing Preview stories, so it's testable
  • The useShowToolbar function can now read that state instead of addons.getConfig

And some noteworthy optimisations and extensions:

  • useShowToolbar doesn't directly read layoutCustomisations from state, but from a dedicated api.getLayoutCustomisations() function so any future architecture change is centralised in one place
  • Ultimately, useShowToolbar is not needed as it's only used in preview.tsx, so the code will be moved back to preview.tsx
  • We can extend the scope of the PR from showToolbar to also include showNav, and showTabs
  • The user-defined functions are responsible for deciding if they honour the layout state where end-users preferences are stored

Ultimately, this update (and possibly others to follow) should let users repurpose Storybook a little bit to include more use cases of documentation, including MDX pages with custom navigation, by only showing the built-in Storybook UI when relevant to their needs.

We also briefly discussed the possibility of introducing a reducer logic in the manager state, where modules could reduce their own part of the manager state. If this had been available, it would have given us more options. The user-defined functions discussed here would obviously be reducer logic that provides more literal types to store in the layoutCustomisations state. Our particular concern was that without a reducer, we can't safely store the outcome of the functions in the manager state, because the functions themselves take the state as a parameter. We'd end up doing multiple consecutive updates or possibly looping infinitely if we did that without a reducer.

I'm going on holiday so there's plenty of time to comment and point out flaws before I start updating the PR. Your feedback is very much welcome :)

@deraw
Copy link

deraw commented May 29, 2024

Hello!
I'm wondering about the state of this issue and StoryBook 8: is the feature now available? Did it changed something in the way it could be implemented?
Thanks!

@Sidnioulz
Copy link
Contributor

Hi @deraw! I need to update my code against the v8 architecture and reopen the PR. It's taking a lot of time because I'm super busy with conferences at the moment :( I'll be able to resume work on this some time in the summer.

@Romainpetit
Copy link

Hey @Sidnioulz any news on that? I just noticed that the SB8 UI has a keyboard shortcut to hide this toolbar, we should be close 🤞

@Sidnioulz
Copy link
Contributor

I may or may not be busy with a Figma CLI so I may or may not be neglecting this PR 😬

The kb shortcut was here since SB 5 or 6, but the problem is storing that you want it disabled by default. We had that logic baked in, but it went through the addon singleton (which needs to go). We had the whole logic down with Norbert to implement this without using the singleton, but I failed at my initial refactor attempt and I have since shamefully forgotten how to handle my sandboxes.

I need to sit down and finish this, I will get back to it as soon as my current project is released.

@Sidnioulz Sidnioulz linked a pull request Nov 2, 2024 that will close this issue
15 tasks
@Sidnioulz
Copy link
Contributor

#29516 is the latest iteration, and it implements most of what we discussed with @ndelangen.

There are a few areas that I'd like to talk to the maintainers about before I finish the doc, especially regarding testing. The features are now testable but the story files where tests had been possible in the past are gone, so I'm not sure what the expectation from the team is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment