Addon-docs: Add eject button to canvas toolbar#29825
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| storyId={getStoryId(getChildProps(children), context)} | ||
| baseUrl="./iframe.html" | ||
| /> |
There was a problem hiding this comment.
style: baseUrl is hardcoded to './iframe.html' which may not be correct in all environments. Consider making this configurable via context or props.
There was a problem hiding this comment.
Does the baseUrl here need similar improvements as #12233?
There was a problem hiding this comment.
Yeah it probably does. If you fix it I can test it out for you!
There was a problem hiding this comment.
@shilman would appreciate if you helped test if this is necessary, I'm not sure it is.
Here's my thinking:
- I see there are some composed storybooks in the test sandbox
- The configuration of my sandbox storybook does not affect those, which makes sense - this feature would have to be enabled in both parent and child storybooks.
- When viewing a child storybook docs page, it renders the child storybook docs page in an iframe, meaning that the links in there should already be correct (they should link to the child storybook story url).
#12233 was needed because the link on the top toolbar is always in the parent storybook, but this is not the case for canvas toolbar.
Granted, I did not manage to get composition working between two sandboxes in localhost, so could not completely test this myself (got some CORS errors, tried vite server proxy but that also failed). If there are issues with composition, I'd appreciate some tips on how to get this working.
|
View your CI Pipeline Execution ↗ for commit c3f22cf
☁️ Nx Cloud last updated this comment at |
|
@shilman any feedback on this one? |
|
Sorry this got assigned to me but I missed it. I'll take a closer look today. Thanks so much @mihkeleidast for the contribution and the ping!!! 🙏 |
shilman
changed the title
There was a problem hiding this comment.
LGTM
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
@mihkeleidast I just tested and this looks great. I did not test the composition use case mentioned in your comment, but I'm pretty sure it will need something similar. Do you know how to test that? If not, I can help you set something up.
FYI, we are in feature freeze right now for 8.6, which should have been released on Tuesday but is slipping. So I'll wait until that goes out before merging.
There was a problem hiding this comment.
2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| <a | ||
| href={getStoryHref(baseUrl, storyId)} | ||
| target="_blank" |
There was a problem hiding this comment.
logic: Add null check before calling getStoryHref since both baseUrl and storyId are optional props
| <Wrapper key="right"> | ||
| <IconButton key="opener" asChild> | ||
| <a | ||
| href={getStoryHref(baseUrl, storyId)} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| title="Open canvas in new tab" | ||
| > | ||
| <ShareAltIcon /> | ||
| </a> | ||
| </IconButton> | ||
| </Wrapper> |
There was a problem hiding this comment.
style: Consider conditionally rendering the eject button only when storyId and baseUrl are available
| <Wrapper key="right"> | |
| <IconButton key="opener" asChild> | |
| <a | |
| href={getStoryHref(baseUrl, storyId)} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| title="Open canvas in new tab" | |
| > | |
| <ShareAltIcon /> | |
| </a> | |
| </IconButton> | |
| </Wrapper> | |
| <Wrapper key="right"> | |
| {baseUrl && storyId && ( | |
| <IconButton key="opener" asChild> | |
| <a | |
| href={getStoryHref(baseUrl, storyId)} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| title="Open canvas in new tab" | |
| > | |
| <ShareAltIcon /> | |
| </a> | |
| </IconButton> | |
| )} | |
| </Wrapper> |
|
@shilman as 8.6 is out now, pinging again to see if you can help me figure out if the composition usecase needs additional work or not - see my previous comment in the thread for details. |
|
Will do @mihkeleidast . Thanks for your patience!! |
|
@shilman It's been a while again, any chance we can move this forward? |
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
LGTM overall, good job @mihkeleidast!
We'd need small clarifications on the change in Preview, and wrapping of the re-added icon with isLoading logic, and we'll be good to go.
| <IconButton key="opener" asChild> | ||
| <a | ||
| href={getStoryHref(baseUrl, storyId)} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| title="Open canvas in new tab" | ||
| > | ||
| <ShareAltIcon /> | ||
| </a> | ||
| </IconButton> | ||
| </Wrapper> |
There was a problem hiding this comment.
This needs to be guarded with isLoading and to return <IconPlaceholder /> when loading.
|
@mihkeleidast I've had a look at the CI beyond the known issue with Angular, and there are a few stories that fail to render on Chromatic, like this one: https://635781f3500dd2c49e189caf-nzzwesbkun.chromatic.com/?path=/story/addons-docs-blocks-components-docspage--loading You should be able to reproduce this in your local Could you please have a look at add a decorator to the relevant stories? Thanks! |
| zoom={(z: number) => setScale(scale * z)} | ||
| resetZoom={() => setScale(1)} | ||
| storyId={getStoryId(children)} | ||
| storyId={!isLoading && childProps ? getStoryId(childProps, context) : undefined} |
There was a problem hiding this comment.
I added a bail-out here for when:
- isLoading=true, as then DocsContext is not defined yet
- if child props cannot be resolved - this is the case for multi-story previews, which makes sense since then we can't resolve a single story URL.
| </> | ||
| )} | ||
| </Wrapper> | ||
| {isLoading ? ( |
There was a problem hiding this comment.
Since storyId is resolved only after loading, changed the placeholder logic here to always show placeholder while loading, and then not rendering the actual button if storyId is not resolved.
|
@Sidnioulz thanks for checking, really should've caught them myself 😅 I changed some logic for the loading state and for multi-story preview, and now CI is passing! I went over the changes in chromatic, most of them seem expected as the eject button is added to the toolbar. Only the "Stories: Different Toolbars" screenshot on Chrome is not rendering correctly there, but that seems not related to my changes as the toolbar change should not affect the canvas height in any way (so maybe it's flaky?). If I can resolve that on my own, let me know, otherwise maybe all is now good here. |
Co-authored-by: Steve Dodier-Lazaro <Sidnioulz@users.noreply.github.com>
I can't find a good reason why this height change is happening, but the Storybook canvas isn't opening. This shouldn't be flaky to the best of my knowledge. Let's see how CI ends up after updating |
|
@mihkeleidast if you test on the "Different Toolbars" autodocs page, what do you see? I see an eject button only for the second story in the Chromatic render. Isn't it supposed to show none? Apart from that, the height problem appears to be gone. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated Docs Preview to derive storyId from child props via DocsContext and getStoryId only when not loading. Enhanced Toolbar to conditionally show an “Open canvas in new tab” button using getStoryHref and ShareAltIcon when baseUrl and storyId are available; shows a placeholder while loading. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant Preview as Docs Preview
participant Ctx as DocsContext
participant Toolbar as Docs Toolbar
participant Nav as Browser
U->>Preview: Render(children, isLoading)
Preview->>Preview: getChildProps(children)
Preview->>Ctx: useContext()
alt not isLoading and childProps
Preview->>Preview: storyId = getStoryId(childProps, context)
else
Preview->>Preview: storyId = undefined
end
Preview-->>Toolbar: props { isLoading, baseUrl, storyId }
Toolbar->>Toolbar: render zoom controls (left)
alt isLoading
Toolbar->>Toolbar: render IconPlaceholder (right)
else isLoading == false and baseUrl && storyId
Toolbar->>Toolbar: href = getStoryHref(baseUrl, storyId)
U->>Nav: Click "Open canvas in new tab"
Nav->>Nav: Open new tab to canvas URL
else
Toolbar->>Toolbar: render nothing (right)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/addons/docs/src/blocks/components/Toolbar.tsx (2)
69-72:preventDefault()is likely unnecessary on button clicks.For
IconButtonwithouthref, default action is already inert; removing noise improves clarity.- onClick={(e: SyntheticEvent) => { - e.preventDefault(); - zoom(0.8); - }} + onClick={() => { + zoom(0.8); + }} ... - onClick={(e: SyntheticEvent) => { - e.preventDefault(); - zoom(1.25); - }} + onClick={() => { + zoom(1.25); + }} ... - onClick={(e: SyntheticEvent) => { - e.preventDefault(); - resetZoom(); - }} + onClick={() => { + resetZoom(); + }}Also applies to: 79-82, 89-92
62-63: Drop unusedkeyprops on static elements.These wrappers aren’t part of an array diff; keys add no value here.
- <Wrapper key="left"> + <Wrapper> ... - <Wrapper key="right"> + <Wrapper>Also applies to: 107-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/addons/docs/src/blocks/components/Preview.tsx(6 hunks)code/addons/docs/src/blocks/components/Toolbar.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
code/addons/docs/src/blocks/components/Toolbar.tsx (1)
code/core/src/components/index.ts (1)
getStoryHref(84-84)
code/addons/docs/src/blocks/components/Preview.tsx (1)
code/core/src/preview-api/index.ts (1)
DocsContext(41-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (9)
code/addons/docs/src/blocks/components/Preview.tsx (6)
2-2: Import organization follows convention.The new imports are appropriately placed and follow the existing import pattern. The context pattern is well-documented for mocking providers in Storybook.
12-13: Imports correctly reference Storybook's context system.Both imports are properly sourced from the correct modules and will support the context-aware story ID resolution functionality.
157-165: Robust helper function for extracting child props.The
getChildPropsfunction correctly handles the single-child case and gracefully returnsnullwhen props cannot be extracted. This aligns with the conditional logic that prevents storyId resolution for multi-story previews.
211-212: Context consumption follows React patterns.The DocsContext is properly consumed using the
useContexthook, enabling access to story metadata needed for ID resolution.
248-248: Efficient child props extraction.Computing
childPropsonce and reusing it for storyId determination is more efficient than multiple traversals and follows good React practices.
262-262: Well-designed conditional story ID resolution.The logic correctly handles multiple scenarios:
- Bails out during loading state when DocsContext is not available
- Prevents storyId resolution when child props cannot be extracted (multi-story previews)
- Only resolves storyId when both context and child props are available
This addresses the past review comments about resolving IDs correctly for non-primary stories.
code/addons/docs/src/blocks/components/Toolbar.tsx (3)
100-120: Eject button rendering and a11y look solid.Guarding with
isLoading, gating onbaseUrl && storyId, usingtarget="_blank"withrel="noopener noreferrer", and addingaria-labelare all correct. Nice work.
53-121: Add lightweight RTL tests for loading vs. eject states.Add three small RTL tests asserting: (1) placeholder icons render when isLoading=true; (2) an anchor with aria-label "Open canvas in new tab" renders when baseUrl+storyId are provided; (3) no anchor renders otherwise.
Add colocated tests for both Toolbar implementations: code/addons/docs/src/blocks/components/Toolbar.tsx and code/core/src/manager/components/preview/Toolbar.tsx (e.g., Toolbar.test.tsx) using @testing-library/react. I can draft them if desired.
108-117: Keep asChild — current usage is correct.
IconButton forwards props to Button and Button sets Comp = Slot when asChild is true, so the anchor child receives the attributes (avoids nested interactive elements). Do not switch to passing href directly unless Button is changed to render an (e.g., add as="a" or href→as conversion).
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
@Sidnioulz Not sure I understand. The second story is configured to show toolbar, so it shows the toolbar for it, and the eject button is on the toolbar. Can you specify what you think is wrong? Do you mean when the sb_theme global is |
uuugh I'm so sorry I'm a bit sleep-deprived 😆 The dots didn't connect in my head. Your code is great! |
shilman
left a comment
shilman
left a comment
There was a problem hiding this comment.
Thanks @mihkeleidast and @Sidnioulz for making this happen and for your patience on getting it merged. Great UI improvement for docs!
4 participants
Closes #25123
What I did
Added the eject button back to Canvas toolbar in docs mode.
I think one future improvement could be to add controls integration to the primary story, as right now it opens with no params applied. But even so, this would be a nice upgrade for our usecases.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Not sure how these blocks are autotested at all. If you can point me in the right direction, I can look into adding tests.
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
yarn startsandbox/react-vite-default-ts/.storybook/preview.tsto enable toolbar on all stories (docs: { canvas: { withToolbar: true } })Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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/coreteam 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>Greptile Summary
Added an "Open canvas in new tab" button to the Canvas toolbar in docs mode, restoring previously missing functionality from issue #25123.
ShareAltIconeject button incode/lib/blocks/src/components/Toolbar.tsxwith proper security attributesEjectPropsinterface inToolbar.tsxto handlestoryIdandbaseUrlpropsPreview.tsxto pass story context and ID to Toolbar componenttarget="_blank"andrel="noopener noreferrer"security attributes💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
Summary by CodeRabbit
New Features
Bug Fixes