Controls: Prevent the save bar from covering the last control#35136
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR fixes a save bar overlap issue in the ControlsPanel by updating component styling and adding a regression test. The component now uses ChangesSave bar overlap fix and regression test
Sequence DiagramsequenceDiagram
participant Decorator as Decorator (set global.CONFIG_TYPE)
participant Host as Scrollable Host Container
participant ControlsPanel
participant SaveBar as Save Bar (`#save-from-controls`)
participant PlayFn as Play Function
PlayFn->>Decorator: set CONFIG_TYPE = DEVELOPMENT
Decorator->>Host: render scrollable host with overflow data
Host->>ControlsPanel: render controls panel with edited args
ControlsPanel->>SaveBar: render save bar (unsaved changes)
PlayFn->>Host: scroll to bottom
PlayFn->>SaveBar: measure getBoundingClientRect
PlayFn->>ControlsPanel: locate last arg row
PlayFn->>PlayFn: assert no vertical overlap between save bar and last row
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/controls/components/ControlsPanel.stories.tsx`:
- Around line 127-132: The test grabs the scroller with a non-null assertion but
doesn't verify it before use; add an assertion like await
expect(scroller).not.toBeNull(); (similar to saveBar) right after querying
scroller on canvasElement so the subsequent scroller.scrollTop =
scroller.scrollHeight; is safe; reference the scroller variable and the
canvasElement.querySelector('[data-testid="panel-scroll"]') call when applying
the change.
- Line 110: Save the current value of global.CONFIG_TYPE before mutating it in
the ControlsPanel story decorator (e.g., const originalConfig =
global.CONFIG_TYPE), set global.CONFIG_TYPE = 'DEVELOPMENT' for the story, then
restore global.CONFIG_TYPE = originalConfig after the story has been
created/rendered (ensure restoration happens in the decorator function path,
e.g., in a finally block or immediately after calling the story function) so the
mutation does not leak to subsequent stories or tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12641327-fc54-4fb3-a02a-0abb66118298
📒 Files selected for processing (2)
code/core/src/controls/components/ControlsPanel.stories.tsxcode/core/src/controls/components/ControlsPanel.tsx
|
@coderabbitai Both addressed in : moved the |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@TheSeydiCharyyev I'll review next week probably, but I remember we had to move some panels from |
|
Did the git archaeology, plus a Firefox check since that's the angle that bit CanvasWrap.
On the min-height→height change you remember: the only one I can find across the repo is CanvasWrap (#33743 / commit 2608c37), which reverted #33290 because I still checked that failure mode against this wrapper in Firefox (Playwright's build — current, not ESR specifically). With I also dropped |
b493084 to
70a2ae3
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 1.29 MB | 1.27 MB | 🎉 -22 KB 🎉 |
| Dependency size | 9.27 MB | 9.27 MB | 🚨 +6 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 21.07 MB | 21.05 MB | 🎉 -28 KB 🎉 |
| Dependency size | 36.41 MB | 36.12 MB | 🎉 -290 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 203 | 203 | 0 |
| Self size | 802 KB | 802 KB | 🎉 -300 B 🎉 |
| Dependency size | 89.52 MB | 89.20 MB | 🎉 -317 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 88.01 MB | 87.69 MB | 🎉 -317 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.08 MB | 1.08 MB | 🎉 -52 B 🎉 |
| Dependency size | 57.48 MB | 57.16 MB | 🎉 -317 KB 🎉 |
| Bundle Size Analyzer | node | node |
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for the thorough check 🙏
Closes #34531
What I did
The "save from controls" bar (shown after you edit a story's args in development) is absolutely positioned at the bottom of the controls panel. The panel reserved room for it with
padding-bottom: 41px, but that padding sat on a wrapper with a fixedheight: 100%. Once the controls overflowed the panel and you scrolled to the bottom, the padding stayed pinned at the wrapper's fixed height — in the middle of the scrolled content — so the last control ended up underneath the bar.Changing the wrapper from a fixed
height: 100%(capped at100vh) tomin-height: 100%keeps it filling a short panel as before, but lets it grow with tall content so the reserved bottom padding always falls after the last control. The last control now clears the save bar.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
A new
SaveBarDoesNotCoverLastControlstory inControlsPanel.stories.tsxrenders an overflowing controls panel with the save bar inside a short scroll container; its play function scrolls to the bottom and asserts the last control's bottom clears the top of the bar. It fails on the previous code and passes with this change.Manual testing
yarn task --task sandbox --start-from auto --template react-vite/default-tsDocumentation
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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Bug Fixes
Tests
SaveBarDoesNotCoverLastControl) that renders an overflow/unsaved-changes scenario in a scrollable container, scrolls to the bottom, and verifies the last control remains visible.