Core: Zoom tool refinements - Hide reset button when value is initial#33635
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced the reset button in the zoom tool with a new styled Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 |
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for taking this on! Could you please apply the fix by not rendering the item instead of hiding it?
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 402 KB | 379 KB | 🎉 -23 KB 🎉 |
| Dependency size | 338 KB | 338 KB | 🎉 -3 B 🎉 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.19 MB | 20.39 MB | 🚨 +201 KB 🚨 |
| Dependency size | 16.52 MB | 16.52 MB | 🎉 -2 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 535 | 535 | 0 |
| Self size | 648 KB | 646 KB | 🎉 -2 KB 🎉 |
| Dependency size | 59.90 MB | 59.93 MB | 🚨 +34 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 92 | 92 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🎉 -383 B 🎉 |
| Dependency size | 22.47 MB | 22.50 MB | 🚨 +33 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 30 KB | 30 KB | 🎉 -126 B 🎉 |
| Dependency size | 23.75 MB | 23.79 MB | 🚨 +33 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 82 | 82 | 0 |
| Self size | 35 KB | 35 KB | 🎉 -106 B 🎉 |
| Dependency size | 20.25 MB | 20.28 MB | 🚨 +33 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 274 | 274 | 0 |
| Self size | 24 KB | 24 KB | 🎉 -86 B 🎉 |
| Dependency size | 44.56 MB | 44.60 MB | 🚨 +34 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 779 KB | 775 KB | 🎉 -4 KB 🎉 |
| Dependency size | 67.37 MB | 67.52 MB | 🚨 +154 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 30 KB | 🎉 -2 KB 🎉 |
| Dependency size | 65.89 MB | 66.09 MB | 🚨 +201 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1.04 MB | 1000 KB | 🎉 -44 KB 🎉 |
| Dependency size | 36.72 MB | 36.92 MB | 🚨 +201 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/preact
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 23 KB | 16 KB | 🎉 -7 KB 🎉 |
| Dependency size | 32 KB | 32 KB | 🎉 -3 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 58 | 57 | 🎉 -1 🎉 |
| Self size | 1.19 MB | 1.23 MB | 🚨 +36 KB 🚨 |
| Dependency size | 13.20 MB | 12.94 MB | 🎉 -264 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
…ing its visibility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/core/src/manager/components/preview/tools/zoom.tsx`:
- Around line 84-93: The input shifts because NumericInput only renders the
after slot when truthy; when value === INITIAL_ZOOM_LEVEL the ActionList.Button
(reset) unmounts and its paddingRight is removed. Fix by always rendering the
after slot as a fixed-width container (or an empty placeholder) inside
NumericInput’s after prop so the space is reserved even when INITIAL_ZOOM_LEVEL
hides the ActionList.Button; update the zoom control (the conditional around
ActionList.Button that uses INITIAL_ZOOM_LEVEL and zoomTo) to render a
placeholder element of equal width instead of null.
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
The edit logic is good, just need to change the code a bit to ensure the menu layout is stable.
Please re-request a review once changes are done so I get notified 🙏
… it for the zoom reset button.
|
547521b |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/components/components/Button/Button.tsx`:
- Around line 142-143: The Button component is forwarding the semantic HTML
hidden attribute to the DOM; change the styling-only prop to a transient prop
(rename hidden → $hidden) so it won't be forwarded. Update the StyledButton
props/interface to use $hidden, update its style logic (visibility rules that
referenced hidden) to use $hidden, add a shouldForwardProp (or rely on transient
prop behavior) to prevent forwarding, and update the Button JSX where you pass
hidden to instead pass $hidden while keeping the public API for consumers (e.g.,
keep the Button prop named hidden and map it internally to $hidden when
rendering StyledButton).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/components/components/Button/Button.tsxcode/core/src/manager/components/preview/tools/zoom.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/manager/components/preview/tools/zoom.tsx
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Could you please make the change only by passing Emotion classes and HTML attributes in zoom.tsx, without any change to Button?
Button is a widely used public component so changes to its public API need to go through a much tougher review process. Here, we don't want to change its API.
4 participants
#33614
What I did
The reset button is no longer displayed when Zoom is in its initial state.
I used
visibility: hiddento ensure the menu width remains consistent even when the button is hidden.Since the styling isn't complex and many parts already use inline styles, I added them inline to make it easier to see which styles apply under which conditions.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
I confirmed that the button disappears when the initial value is set.
I also confirmed that the menu width does not change based on the button's visibility, as intended.
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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Refactor