storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.46k stars 9.28k forks source link

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

Open noranda opened 1 year ago

noranda commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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
  2. 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
  3. 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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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:

https://github.com/storybookjs/storybook/blob/bdbd33592b8d38231406c43fa6b72c9c57c5f625/code/lib/manager-api/src/lib/stories.ts#L159-L160

We could probably do something similar with reading toolbar options here-ish: https://github.com/storybookjs/storybook/blob/bdbd33592b8d38231406c43fa6b72c9c57c5f625/code/ui/manager/src/components/preview/toolbar.tsx#L136-L138

Sidnioulz commented 1 year ago

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

Sidnioulz commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

And some noteworthy optimisations and extensions:

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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

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.