Manager: Ensure reset item only appears in globals toolbar when specified #33276
Conversation
|
View your CI Pipeline Execution ↗ for commit 677bc88 ☁️ Nx Cloud last updated this comment at |
| tooltip={ariaLabel} | ||
| resetLabel={resetLabel} | ||
| onReset={() => updateGlobals({ [id]: '_reset' })} | ||
| onReset={resetItem ? () => updateGlobals({ [id]: resetItem?.value }) : undefined} |
There was a problem hiding this comment.
Thanks @mrginglymus! The conditional fix LGTM, I'll check the potential effects of setting undefined on URL query params and globals persistence as I suspect there was a reason why _reset was used.
There was a problem hiding this comment.
We do have a side effect that you can test on your monorepo.
If you reset the locale global, it lingers in the URL without a value: http://localhost:6006/?path=/story/core-backgrounds-globals--set&globals=locale:.
In contrast, the viewport global does not linger. That's because global toolbars' normalisation code forced the reset option to have a '' value, which makes little sense. Global toolbar types do not allow for a default value, so we can safely set the reset item's value to undefined for now (and change it to match any default value in the future).
I've pushed the relevant commit to your branch.
Sidnioulz
changed the title
📝 WalkthroughWalkthroughChanges to toolbar reset behavior: modifications to how the reset handler is wired in ToolbarMenuSelect and how reset item values are normalized. The reset behavior is now conditional based on resetItem presence rather than always using a static value. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
✨ Finishing touches
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 (1)
code/core/src/toolbar/components/ToolbarMenuSelect.tsx (1)
101-101: LGTM! Conditional onReset correctly prevents unwanted reset option.The conditional logic ensures
onResetis only provided when a reset item is explicitly configured, which fixes the core issue. The handler correctly usesresetItem.value(which isundefinedfrom the normalization logic) instead of a hardcoded'_reset'string.Minor note: The optional chaining
resetItem?.valueis redundant since we're already in a branch whereresetItemis truthy. You could useresetItem.valuedirectly, but the defensive approach is harmless.If desired, you can simplify to:
- onReset={resetItem ? () => updateGlobals({ [id]: resetItem?.value }) : undefined} + onReset={resetItem ? () => updateGlobals({ [id]: resetItem.value }) : undefined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/toolbar/components/ToolbarMenuSelect.tsx(1 hunks)code/core/src/toolbar/utils/normalize-toolbar-arg-type.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/toolbar/utils/normalize-toolbar-arg-type.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/toolbar/utils/normalize-toolbar-arg-type.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/toolbar/utils/normalize-toolbar-arg-type.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/toolbar/utils/normalize-toolbar-arg-type.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/toolbar/utils/normalize-toolbar-arg-type.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsx
⏰ 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 (2)
code/core/src/toolbar/utils/normalize-toolbar-arg-type.ts (1)
22-26: LGTM! Reset value correctly set to undefined.The change ensures reset items use
undefinedas their value instead of a hardcoded string like'_reset'. This aligns with the PR objective and works correctly with the conditionalonResethandler in ToolbarMenuSelect.code/core/src/toolbar/components/ToolbarMenuSelect.tsx (1)
57-58: LGTM! Reset item lookup is correct.The code properly identifies the reset item from the toolbar items and extracts its label for display.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 39 | 39 | 0 |
| Self size | 20.54 MB | 20.52 MB | 🎉 -16 KB 🎉 |
| Dependency size | 16.41 MB | 16.41 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 173 | 173 | 0 |
| Self size | 774 KB | 774 KB | 🎉 -505 B 🎉 |
| Dependency size | 67.61 MB | 67.60 MB | 🎉 -16 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 166 | 166 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 66.19 MB | 66.17 MB | 🎉 -16 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 40 | 40 | 0 |
| Self size | 1000 KB | 1000 KB | 🚨 +73 B 🚨 |
| Dependency size | 36.94 MB | 36.93 MB | 🎉 -16 KB 🎉 |
| Bundle Size Analyzer | node | node |
valentinpalkovic
changed the title
3 participants
Closes #33271
What I did
The
Selectcomponent would add a 'reset' option ifonResetwas specified. TheToolbarMenuSelectcomponent was unconditionally providing this callback.The fix is to only pass in the
onResethandler if a reset item has been specified by the user.In addition, the reset handler is called with the reset value, instead of the static string
_reset.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
In the default storybook, note that the theme global no longer has a reset option:
but the locale one does
and clicking it now sets the locale to
undefinedinstead of'_reset'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
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.