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: Sidebar context menu addon API #29557

Open
wants to merge 41 commits into
base: next
Choose a base branch
from

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Nov 6, 2024

What I did

  • allow custom link in menu list component
  • add a new method on the testProvider addon type for adding a contextMenu rendering
  • adjust the Tree component to render a context menu, including statuses & contextMenu renderings from testProviders
  • hoist the state of the sidebarBottom into a state module in manager-api

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

I'll be adding this section as soon as we have alignment on this feature's implementation

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for 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-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

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

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 78.2 MB 78.2 MB 427 B 1.21 0%
initSize 143 MB 144 MB 172 kB 15.54 0.1%
diffSize 65.2 MB 65.3 MB 172 kB 18.89 0.3%
buildSize 6.88 MB 7.03 MB 156 kB 2204.62 2.2%
buildSbAddonsSize 1.51 MB 1.51 MB 0 B 0.23 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.9 MB 2.06 MB 156 kB 2229.16 🔺7.6%
buildSbPreviewSize 271 kB 271 kB 0 B - 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.88 MB 4.04 MB 156 kB 2203.09 3.9%
buildPreviewSize 3 MB 3 MB 109 B Infinity 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 7.2s 24.4s 17.1s 1.73 🔺70.4%
generateTime 21s 27.7s 6.6s 1.13 24.1%
initTime 14.8s 20.3s 5.5s 3.85 🔺27.2%
buildTime 8s 8.7s 781ms -0.09 8.9%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 5.6s 4.9s -637ms -1.3 🔰-12.8%
devManagerResponsive 3.5s 3.2s -354ms -1.02 -11%
devManagerHeaderVisible 552ms 564ms 12ms -0.03 2.1%
devManagerIndexVisible 589ms 637ms 48ms 0.07 7.5%
devStoryVisibleUncached 795ms 1.1s 332ms 0.26 29.5%
devStoryVisible 587ms 598ms 11ms -0.24 1.8%
devAutodocsVisible 559ms 376ms -183ms -1.87 🔰-48.7%
devMDXVisible 473ms 487ms 14ms -0.41 2.9%
buildManagerHeaderVisible 547ms 512ms -35ms -0.81 -6.8%
buildManagerIndexVisible 570ms 522ms -48ms -0.88 -9.2%
buildStoryVisible 538ms 506ms -32ms -0.86 -6.3%
buildAutodocsVisible 431ms 421ms -10ms -0.73 -2.4%
buildMDXVisible 420ms 443ms 23ms -0.12 5.2%

Greptile Summary

Added context menu functionality to Storybook's sidebar, allowing addons to inject custom links and actions through a new experimental test provider API.

  • Added contextMenu property to Addon_TestProviderType in code/core/src/types/modules/addons.ts for custom menu items
  • Implemented useContextMenu hook in code/core/src/manager/components/sidebar/Tree.tsx to manage hover/click states
  • Added test provider state management in code/core/src/manager-api/modules/experimental_testmodule.ts
  • Modified TooltipLinkList in code/core/src/components/components/tooltip/TooltipLinkList.tsx to support custom content rendering
  • Added example implementation in test addon (code/addons/test/src/manager.tsx) for running tests via context menu

Copy link

nx-cloud bot commented Nov 6, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 7e43ecd. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen ndelangen self-assigned this Nov 11, 2024
@ndelangen ndelangen marked this pull request as ready for review November 13, 2024 09:19
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

14 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings | Greptile

code/addons/test/src/manager.tsx Outdated Show resolved Hide resolved
code/core/src/manager/components/sidebar/Tree.tsx Outdated Show resolved Hide resolved
code/core/src/manager/components/sidebar/Tree.tsx Outdated Show resolved Hide resolved
code/core/src/manager/components/sidebar/Tree.tsx Outdated Show resolved Hide resolved
@@ -133,7 +133,7 @@ const WithTooltipPure = ({
{/* @ts-expect-error (non strict) */}
{typeof tooltip === 'function' ? tooltip({ onHide: () => onVisibleChange(false) }) : tooltip}
</Tooltip>
);
) : null;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an optimization that makes a lot of sense to me...
Why call a function which we will not use the returned JSX of?

Comment on lines +182 to 190
api.on(TESTING_MODULE_CRASH_REPORT, onCrashReport);
api.on(TESTING_MODULE_RUN_ALL_REQUEST, clearState);
api.on(TESTING_MODULE_PROGRESS_REPORT, onProgressReport);

return () => {
api.getChannel()?.off(TESTING_MODULE_CRASH_REPORT, onCrashReport);
api.getChannel()?.off(TESTING_MODULE_PROGRESS_REPORT, onProgressReport);
api.getChannel()?.off(TESTING_MODULE_RUN_ALL_REQUEST, clearState);
api.off(TESTING_MODULE_CRASH_REPORT, onCrashReport);
api.off(TESTING_MODULE_PROGRESS_REPORT, onProgressReport);
api.off(TESTING_MODULE_RUN_ALL_REQUEST, clearState);
};
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm considering moving this code into the state module as well.

Right now, every time testProviders changes (which happens a LOT due to that's where the state is), we unsubscribe and resubscribe.

It's technically possible to miss events during this short period.

Comment on lines +480 to +486
contextMenu?: (
options: {
context: API_HashEntry;
state: Addon_TestProviderState<Details>;
},
components: { ListItem: typeof ListItem }
) => ReactNode;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the added API to Addon_TestProviderType.

I'm personally a bit on the fence about the components bit, as the user is likely to need more components anyway. Adding more is an option, but @JReinhold has stronger opinions on this than I do.

@storybook-pr-benchmarking
Copy link

storybook-pr-benchmarking bot commented Nov 13, 2024

Package Benchmarks

Commit: 7e43ecd, ran on 14 November 2024 at 22:09:31 UTC

The following packages have significant changes to their size or dependencies:

@storybook/core

Before After Difference
Dependency count 46 46 0
Self size 19.08 MB 19.24 MB 🚨 +169 KB 🚨
Dependency size 14.29 MB 14.29 MB 0 B
Bundle Size Analyzer Link Link

storybook

Before After Difference
Dependency count 47 47 0
Self size 22 KB 22 KB 0 B
Dependency size 33.37 MB 33.53 MB 🚨 +169 KB 🚨
Bundle Size Analyzer Link Link

sb

Before After Difference
Dependency count 48 48 0
Self size 1 KB 1 KB 0 B
Dependency size 33.39 MB 33.56 MB 🚨 +169 KB 🚨
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 390 390 0
Self size 482 KB 482 KB 0 B
Dependency size 74.43 MB 74.60 MB 🚨 +169 KB 🚨
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 270 270 0
Self size 612 KB 612 KB 0 B
Dependency size 64.42 MB 64.59 MB 🚨 +169 KB 🚨
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 105 105 0
Self size 1.11 MB 1.11 MB 0 B
Dependency size 42.37 MB 42.54 MB 🚨 +169 KB 🚨
Bundle Size Analyzer Link Link

Comment on lines 74 to 85
if (options?.selection) {
const listOfFiles: string[] = [];

// TODO: get actual list and emit, this notification is for development purposes
fullAPI.addNotification({
id: 'testing-module',

content: {
headline: 'Running tests',
subHeadline: `Running tests for ${listOfFiles} stories`,
},
});
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to implement the subset-testProvider running here @JReinhold

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