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: Fix custom tabs not showing in the manager #25792

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jan 29, 2024

Closes #25322
Closes: #25758

What I did

  • change Tabs type addon to always render via a query parameter &tab={tabId}
  • Fix memoization of multiple layers

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

  • Open storybook
  • expect to see no tabs
  • Add a custom tab type addon in .storybook/manager.ts
  • Restart + open storybook
  • expect the "canvas"-tab + any tabs manually configured to show up
  • Clicking between both tabs show should you the content of canvas / tabs
  • The URL should not contain a tab=... when the "Canvas" tab is active
  • The active tan should be highlighted with a bar
  • Tools should only be shown on the "Canvas" tab

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/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>

@ndelangen ndelangen linked an issue Jan 29, 2024 that may be closed by this pull request
@ndelangen ndelangen changed the title Core: UI manager Tabs addon not showing Core: Fix UI manager Tabs addon not showing Jan 29, 2024
@ndelangen ndelangen added the bug label Jan 29, 2024
@JReinhold JReinhold changed the title Core: Fix UI manager Tabs addon not showing UI: Fix custom tabs not showing in the manager Jan 30, 2024
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Looks good to me, only a few minor comments. Also needs a migration note mentioning the URL for tabs have now changed, and that tabId is now passed into match and how to use it.

Will you do a Documentation Request too?

code/ui/manager/src/components/preview/Preview.tsx Outdated Show resolved Hide resolved
code/ui/manager/src/components/preview/Toolbar.tsx Outdated Show resolved Hide resolved
@ndelangen
Copy link
Member Author

@JReinhold I added the migration notes, and submitted a documentation update request 👍 Thank you for the reminder!

Copy link

socket-security bot commented Jan 31, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@playwright/test@1.41.1 None 0 25.3 kB yurys

🚮 Removed packages: npm/@playwright/test@1.36.0, npm/playwright-core@1.36.0, npm/playwright@1.36.0

View full report↗︎

MIGRATION.md Outdated
@@ -386,6 +387,46 @@

## From version 7.x to 8.0.0

### Tab addons are now routed to a query parameter
Copy link
Member

Choose a reason for hiding this comment

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

This migration note should describe the URL structure before the change and after. The current content should be moved down to "Addon author changes" since it's really not end user facing.

Copy link
Member

Choose a reason for hiding this comment

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

Also we can merge this PR as is and fix it in a follow-on PR, since time is tight

@kylegach kylegach mentioned this pull request Feb 1, 2024
4 tasks
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM if you split up that migration note after merge :D

@ndelangen ndelangen merged commit 98863cb into next Feb 1, 2024
57 of 58 checks passed
@ndelangen ndelangen deleted the jeppe/25322-bug-main-section-disappears-when-navigating-to-addon-tab branch February 1, 2024 08:45
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.

[Bug]: Custom tabs not shown in tool bar [Bug]: Main section disappears when navigating to addon tab
3 participants