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

Return to filtering toolbars in docs mode, but don't filter menu #19959

Merged
merged 3 commits into from
Dec 2, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Nov 25, 2022

Issue: Toolbars regressed with changes for docs 2.

What I did

Took steps to restore prior behaviour. I:

  1. Ensure the old filtering behaviour applied in docs mode. This led to the zoom buttons and some addons being removed from the global toolbar in docs mode.
  2. Removed the eject button from the docs/canvas toolbar (as discussed).

What I didn't do

  1. Any new behaviour around the toolbar, that will come later.
  2. Resolved the issue with the backgrounds toolbar not appearing (globally). An explanation:

The issue is that backgrounds (and I suspect many other addons) call useParameters():

const backgroundsConfig = useParameter<BackgroundsParameter>(
BACKGROUNDS_PARAM_KEY,
DEFAULT_BACKGROUNDS_CONFIG
);

If that returns undefined the toolbar does not show (note the grid toolbar item always shows, which is a bit inconsistent 🤷 ).

useParameters / api.getCurrentParameters()

This used to be the current story’s parameters, which made sense in 6.x where you either rendered a docs page for a story or a "docs only" story (standalone docs page).

This is not the case in docs2, where we are rendering docs entries, not stories.

It might make sense (for now) for api.getCurrentParameters() to return the parameters of the first story of the component the docs is rendering for. However, there are two problems with this:

  1. True "Standalone" docs entries (e.g. Introduction.mdx) do not have a component/any stories attached. Really I guess what you'd want is the project parameters to be returned here, but we don't know what they are in the manager in SSv7.

  2. Even for docs entries that are connected to a component have an issue because we don't "prepare" a story (i.e. send its parameters over to the manager) until it is selected.

  • However we could change this to happen when it is rendered instead.

However, I am loathe to do that to solve 2 unless we figure out what to do about 1.

Other solutions

Alternative 1

We could make api.getCurrentParameters() return undefined if a docs entry is selected (the current behaviour). However for many addons this would mean they'd have to hide themselves because they wouldn't know how they are configured.

Alternative 2

We could make api.getCurrentParameters() return the project parameters when a docs entry is selected. This would mean that any component-level configuration of an addon wouldn't apply in docs mode though. This might be relatively simple however.

Alternative 3

If we moved addons to the story toolbar, it would mean:

    1. would no longer be an issue as we just wouldn't show addons for these entries.
  • We could maybe figure out a way to make useParameters() actually figure out which story the addon is rendered for via a context provider. (we'd still need to figure out the preparing thing). So 2. could be solved neatly, without the hack of "find the first story for the component connected to the docs entry".

However I don't think this is feasible because what the addon actually does based on the parameters it totally arbitrary and likely not tied to the story id. So there's no way to ensure addons would only act on a single story. We could of course update the essentials to fix this, but many community addons would break.

How to test

Try the following for a story, a docs page for a component, a unlinked docs page:

  • check global toolbar
  • check story toolbar
  • try opening/closing the sidebar and make sure menu toolbar appears.

@JReinhold
Copy link
Contributor

Alternative 2

We could make api.getCurrentParameters() return the project parameters when a docs entry is selected. This would mean that any component-level configuration of an addon wouldn't apply in docs mode though. This might be relatively simple however.

I like this solution, at least for standalone docs. I think it would be very unexpected if the toolbar would magically behave based on the first Story in the docs. It would make more sense if it just behaved like there was no story at all, even though that might be inconvenient in some edge cases.

Except for a story's auto docs page, I would expect that to pick up the parameters from the stories' meta. But I don't know how feasible that is.

Alternative 3

...
However I don't think this is feasible because what the addon actually does based on the parameters it totally arbitrary and likely not tied to the story id. So there's no way to ensure addons would only act on a single story. We could of course update the essentials to fix this, but many community addons would break.

I think this is the exactly what @shilman has tried to warn us about in this discussion, that it would be optimal to have the toolbar in each canvas, however they would probably not contain their actions to the canvas, but to the whole document.

Short-term I think Alternative 2 is a good approach.
Long-term I think Alternative 3 is the way we want to go, so addons are tied to canvases and not docs, but it might require a breaking change in v8.

How does the addon toolbar interact with stories in iframe mode within docs? Eg. the measure addon, I would expect it to currently measure everything in the docs (not just the canvases), but I would be (positively) surprised if it also worked for stories in iframe mode?

@tmeasday
Copy link
Member Author

tmeasday commented Nov 27, 2022

I think it would be very unexpected if the toolbar would magically behave based on the first Story in the docs.

I was only suggesting it would be the first story for the relevant component in cases 2&3. I don't think there's any argument that the best thing for 1. is to use the project parameters (or none at all).

It would make more sense if it just behaved like there was no story at all, even though that might be inconvenient in some edge cases.

Cases 2&3 aren't really edge cases though.

Agree, I was talking about case 1 here.

Except for a story's auto docs page, I would expect that to pick up the parameters from the stories' meta. But I don't know how feasible that is.

From a user perspective, I don't think 2&3 are all that different, so whatever we do for case 3 we should probably do for case 2.

Agree.

@tmeasday tmeasday marked this pull request as ready for review December 1, 2022 10:00
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.

2 participants