UI: Add ability to programmatically hide the toolbar

[Sidnioulz]

Closes #22081

What I did

Following conversations on the issue, I:

• Added a manager option to add a function that controls the toolbar visibility
• Edited the preview code to handle it
• Added stories to test/document toolbar visibility behaviour
• Edited documentation to mention the new function

I have some doubts over the following:

• I had to seriously hack the story showing the toolbar using my new option, because addons is global and the FakeProvider can't be made to change it
• I am not sure about documentation standards (tone of voice, glossary, etc)
• I chose not to document all of layout in my function parameters as I believe some props to be irrelevant, or possibly legacy (showTabs)
• I didn't merge my new documentation into the Toolbars and Globals page because it's already cramped, and answers to creating custom menus; it felt more logical to have a page dedicated to the Toolbar UI element with the same documentation style as the sidebar, etc

How to test

1. Customise code/ui/.storybook/manager.tsx with the code snippet below
2. Run yarn storybook:ui
3. Open Storybook in your browser
4. Notice how the toolbar is gone

addons.setConfig({
  toolbar: { showToolbar: () => false },
});

Checklist

• Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
• Make sure to add/update documentation regarding your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
• Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

[kylegach]

If the value is a string, then that should be represented here:

Suggested change

	| **path** | String | Path to the page being displayed | `/story/components-button--default` |
	| **path** | String | Path to the page being displayed | `'/story/components-button--default'` |

Applies to all String type possible values.

[Sidnioulz]

Thanks, I wasn't sure about that. I inspired myself from existing docs, so is it expected that some docs show strings without single quotes? Is it just debt?

[kylegach]

Oh, apparently. 😅

If you point out the ones you're aware of to me, I'll make sure they're addressed. 🙏

[Sidnioulz]

• docs/addons/addons-api.md
• docs/configure/features-and-behavior.md
• docs/essentials/viewport.md (device type)
• docs/writing-stories/naming-components-and-hierarchy.md (locale)

[Sidnioulz]

I've used a quick and dirty code search, could have missed some obviously.

[kylegach]

Hi, @Sidnioulz! I reviewed this from a documentation perspective only.

First, thank you for not only updating the docs, but creating a new and comprehensive docs page. 👏

Second, I left a number of comments and suggestions for you to consider. Feel free to ping me if you have any questions!

[tmeasday]

Great stuff here, I have some suggestions/tweaks

[tmeasday]

Wouldn't this affect the true manager rather than the mocked one provided by the ManagerProvider above?

[Sidnioulz]

Absolutely. This is unavoidable for now as the addons store is a global singleton. As the store setConfig function only adds to the current store, I need to actively remove the function for the story to have reasonable side effects (but side effects nonetheless). If I had had a default state for that store, I'd have had much worse side effects.

I see two ways of resolving this:

• Providing addons through the manager React context instead of as a singleton (massively breaking change)
• Editing the addon store to add a save/restore mechanism for tests, either through
  • Explicit functions, one to snapshot/save the state, and one to restore the state to the last saved snapshots, which makes it usable by end users with all the implications
  • A wrapper function for arbitrary code, that saves upon entering and restores upon leaving, which would be more specifically for test stories to use as a decorator

Of course I might have missed that there's already a method to solve my issue in the codebase.

[tmeasday]

Ok, wow, I didn't realise that. That's kind of a mess.

Providing addons through the manager React context instead of as a singleton (massively breaking change)

It's pretty clear we should do this sooner rather than later. I don't know if it's a breaking change to start reading addon info from a provider in a new spot we are accessing it (i.e. in this PR's useShowToolbar). But understand if it feels out of scope of this PR.

It could just be like:

const useShowToolbar = (showToolbar: boolean) => {
  const config = useContext(AddonsProvider)
}

@ndelangen any thoughts on this?

--

I have another suggestion, if the above is too much -- which is to make a test-only prop to the PreviewComponent for the result of addons.getConfig().

So the code might look something like:

const useShowToolbar = (showToolbar: boolean, overrideAddonsConfig: Record<string, any>) => {
  const config = overrideAddonsConfig || addons.getConfig()
}

--

In lieu of the either above, I'm not a big fan of a story that messes with the storybook that renders it, that seems like asking for trouble (even when you make sensible efforts to undo those changes). I'd probably advocate something that gives us less coverage 🤷

[Sidnioulz]

It would be a breaking change in the sense that users expect it to be global. They expect to be able to call addons.setConfig and have the changes reflected.

If my code used the addons store through a manager context instead, with the underlying assumption that the store content is unique to the manager and separated from the global singleton, then using the current global API could sometimes fail to result in a change to the store that the toolbar reads from.

---

That's why I'd prefer to have a pattern built into the store. It lets us build "social unit tests" safely so long as Storybook isn't rendering two stories at the same time (safe assumption to make?).

Snapshotting the store state and restoring it is easy to build within the store. There would be side effects if/when you parallelise story tests, though, so then the pattern could not be used as is. Stories like mine could be flagged to not be parallelised to avoid this issue.

---

EDIT: Everything below is wrong. It cannot work because some addon config is read in a manager context with no knowledge of what's being rendered; if there ever was concurrent rendering of stories, then the toolbar UI itself would too render concurrent stories

Alternatively, detaching the store state could work and could handle the problems of concurrent rendering or concurrent testing:

• Instead of snapshotting the story state and restoring it, the addons store could create a detached, uniquely identified store copy when asked
• In stories that use the addon store, we'd create detached copies identified by the story id
• addons.setConfig would need an extra optional param; when passed, it uses the detached copy with the matching id, or reverts to the main store
• Internally within SB, where we want support for story tests with addons, we pass the story id to our addons.setConfig calls, which results in:
  • In a test story context, the detached store is used and the main store is not impacted
  • In a production environment, the main store is always used

// preview.stories.tsx
export const WithToolbarHiddenFromConfig = () => {
  useEffect(() => {
    addons.detach('story-id-goes-here-somewhat')
    addons.setConfig({
      toolbar: {
        showToolbar() {
          return false;
        },
      },
    }, 'story-id-goes-here-somewhat');

    return () => {
      addons.cleanup('story-id-goes-here-somewhat');
    };
  }, []);
  return <Preview {...previewProps} />;
};

Ultimately this is a variation of what you're proposing, though done at the store level:

• I honestly don't see how else I can pass this test prop to the preview component for it to then pass to the toolbar so it feels easier that way for me
• There's the mild advantage that this store change could be used for more than testing a specific component, or even more than writing tests in general

[tmeasday]

@Sidnioulz I think maybe you misunderstood my intention:

It would be a breaking change in the sense that users expect it to be global. They expect to be able to call addons.setConfig and have the changes reflected.

If my code used the addons store through a manager context instead, with the underlying assumption that the store content is unique to the manager and separated from the global singleton, then using the current global API could sometimes fail to result in a change to the store that the toolbar reads from.

What I was thinking is simply that either similar to, or even part of [1] the ManagerProvider, we wrap the whole manager in a context provider that provides the existing global singleton.

Then, in any new code (like this PR), we read from the context rather than the global singleton directly. It will amount to the same thing (as reading directly) in the "real" code, but allow mocking that context in stories.

Then, in stories (of that new code) we can simply wrap the story in a different provider that mocks the addons API.

[1] We could possibly add the addon config to the Combo type:

storybook/code/lib/manager-api/src/index.tsx

Lines 137 to 140 in 1dc1ed5

	export interface Combo {
	api: API;
	state: State;
	}

[Sidnioulz]

Ah! I did miss your point indeed 😆

As the story renders a manager via a decorator, we could indeed mock it. I initially tried to do that but couldn't find a way to reach the addon singleton that way. If I read addons from the context in my toolbar code, it becomes possible indeed. As the manager is within the story, it's well isolated for concurrent tests/rendering.

I'll give it a go ASAP, thanks Tom!

[tmeasday]

Awesome. @ndelangen is away this week but is sort of the expert on the manager architecture so he'll likely want to weigh in next week also.

[ndelangen]

So the issue here is the fact that this API emits an event, that's send across the channel.

storybook/code/lib/manager-api/src/lib/addons.ts

Lines 152 to 161 in c7028c7

	setConfig = (value: Addon_Config) => {
	Object.assign(this.config, value);
	if (this.hasChannel()) {
	this.getChannel().emit(SET_CONFIG, this.config);
	} else {
	this.ready().then((channel) => {
	channel.emit(SET_CONFIG, this.config);
	});
	}
	};

So the act of changing this applies to both the manager api instance running in the preview, but also the manager api instance running in the manager.

Calling the underlaying API directly would not have this problem:

storybook/code/lib/manager-api/src/modules/layout.ts

Lines 288 to 290 in 13843a4

	provider.channel.on(SET_CONFIG, () => {
	api.setOptions(merge(api.getInitialOptions(), persisted));
	});

I think that would solve this issue, but at the cost of not using the "public API".

[Sidnioulz]

Thanks Norbert!

So, going forward:

• I'll rewrite the preview tests and FakeProvider to inject the addons store directly
• In my toolbar code, I'll retrieve the API via my React Context Consumer
• Once I have it, I'll call setOptions to avoid the side-effect of emitting events on the channel

Is that correct?

[ndelangen]

@Sidnioulz I'm not so sure.
I'm not so sure there's a need for useShowToolbar.tsx to exist.

The setConfig API sets state in the layout field in the global storybook state:

storybook/code/ui/manager/src/containers/preview.tsx

Lines 25 to 42 in d772d38

	const mapper = ({ api, state }: Combo) => {
	const { layout, location, customQueryParams, storyId, refs, viewMode, path, refId } = state;
	const entry = api.getData(storyId, refId);
	return {
	api,
	entry,
	options: layout,
	description: getDescription(entry),
	viewMode,
	path,
	refs,
	storyId,
	baseUrl: PREVIEW_URL || 'iframe.html',
	queryParams: customQueryParams,
	location,
	};
	};

This initial state is set (but also persisted to/from localStorage):

storybook/code/lib/manager-api/src/modules/layout.ts

Lines 281 to 292 in 13843a4

	const persisted = pick(store.getState(), 'layout', 'selectedPanel');
	return {
	api,
	state: merge(api.getInitialOptions(), persisted),
	init: () => {
	api.setOptions(merge(api.getInitialOptions(), persisted));
	provider.channel.on(SET_CONFIG, () => {
	api.setOptions(merge(api.getInitialOptions(), persisted));
	});
	},
	};

I'm wondering if we can just amend the container component to pull function and pass it to the Preview component. If that's possible we don't need a new context at all.

[tmeasday]

Can we pass the whole entry in here so you can read tags etc?

[Sidnioulz]

Sure. Is there a way I can record that the toolbar doc will need updating once tags are out, then?

[tmeasday]

@kylegach is there a good place to track this sort of thing?

[Sidnioulz]

Closing in favour of #29516.