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: Add an experimental API for adding sidebar bottom toolbar #23778

Merged
merged 16 commits into from
Aug 24, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Aug 9, 2023

Telescoped off: #23722

What I did

I added a new type of addon that will be injected into a new bottom bar of the sidebar.
It looks like this:
Screenshot 2023-08-10 at 00 13 37

How to test

Here's how to use it:

import React from 'react';
import { addons, types } from '@storybook/manager-api';

addons.register('my/design-addon', () => {
  addons.add('my/design-addon', {
    type: types.experimental_BOTTOM,
    id: 'my/design-addon/panel',
    render: () => <div>HI</div>,
  });
});

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"]

🦋 Canary release

This pull request has been released as version 0.0.0-pr-23778-sha-fa3b4d8f. Install it by pinning all your Storybook dependencies to that version.

More information
Published version 0.0.0-pr-23778-sha-fa3b4d8f
Triggered by @ndelangen
Repository storybookjs/storybook
Branch norbert/add-sidebar-footer-slot
Commit fa3b4d8f
Datetime Wed Aug 23 13:24:12 UTC 2023 (1692797052)
Workflow run 5951943981

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=23778

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Seems pretty neat!

Couple comments/qns:

  1. Naming: maybe it should be SIDEBAR_BOTTOM? I feel like BOTTOM is a bit ambiguous and we might regret it later?

  2. Are we thinking down the track we might remove this API in favour of a tighter API that only allows you to put sidebar filtering buttons in this space? If so, we should make it clear in the jsdoc that it's intended to be a temporary API.

  3. How does it work if two addons put buttons in the space and then we turn a filter on? Wouldn't we want the space to only display the "clear" button of a single addon at that point? How are we thinking about that in this more general API? (This is a reason why I think a more constrained API might be a better idea)

@ndelangen
Copy link
Member Author

@tmeasday I think this slot is not JUST for filtering buttons. That's what we'll use it for, but not necessarily what other addon creators will be using it for.

Adding and disabling filter functions is simply replacing filter functions with different ones.
To storybook there is no concept of enabling or disabling filter functions.

@ndelangen ndelangen requested a review from tmeasday August 10, 2023 11:28
@tmeasday
Copy link
Member

tmeasday commented Aug 11, 2023

I guess as long as we are OK with it being a bit busy when multiple buttons are there, the design (which looks something like this), probably makes it clear how to clear the filter:

image
image

Adding and disabling filter functions is simply replacing filter functions with different ones.

This is the wrong PR to talk about how you go about replacing but I'm not sure I understand how that API works exactly.

So I guess filters are going to be additive? We might eventually have multiple toggleable buttons in there that each apply their own filter? I guess that makes sense, but I still feel like there's a pattern here that we might want to make more explicit

(Pattern being: add a toggle button to the bottom sidebar area that when active enables a filter and when inactive disables it).

@ndelangen
Copy link
Member Author

I guess as long as we are OK with it being a bit busy when multiple buttons are there

I'd seem wise for addons to be conservative when adding items in this slot. But it's not something we can really control.

go about replacing but I'm not sure I understand how that API works exactly.

Each filter-function has a unique id, addons know the id they injected, so they can replace that one.
So injected a new filter-function, and replacing an existing one, works exactly the same way.

So I guess filters are going to be additive?

When multiple filters are active, if 1 filter removes an item, it's gone, even if the other filter would allow it.
And you can have infinitely many filters.

(Pattern being: add a toggle button to the bottom sidebar area that when active enables a filter and when inactive disables it).

Let's generalize when the pattern has more then 1 use.

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

It seems like the bottom bar is added quite fast, while the sidebar is still loading. Is that OK? Additionally as result you get an unexpected layout shift, as the sidebar section only moves to the bottom once the sidebar is fully loaded

does not feel like a blocker, but wanted to let you know

code/ui/.storybook/manager.tsx Outdated Show resolved Hide resolved
@ndelangen
Copy link
Member Author

@yannbf I'm now no longer rendering the bottom bar during loading.

Base automatically changed from norbert/sidebar-filter-api to next August 21, 2023 11:43
@yannbf
Copy link
Member

yannbf commented Aug 22, 2023

That's pretty good @ndelangen! There is still some layout shift, but I think that's quite minor and we can do something about it at a later iteration

@ndelangen ndelangen merged commit 1f18e29 into next Aug 24, 2023
@ndelangen ndelangen deleted the norbert/add-sidebar-footer-slot branch August 24, 2023 12:24
@github-actions github-actions bot mentioned this pull request Aug 25, 2023
15 tasks
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.

3 participants