UI: Fix overflowing addon panel when hidden on initial render #33450
Conversation
|
View your CI Pipeline Execution ↗ for commit 6e71781
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughRefactors the top-level manager layout from CSS grid to a wrapper-driven flex composition, introduces Changes
sequenceDiagram
autonumber
participant Layout as Layout (Layout.tsx)
participant Sidebar as SidebarContainer
participant Wrapper as ContentPanelWrapper
participant Content as ContentContainer/Pages
participant Panel as PanelContainer
Note over Layout: Desktop render (flex flow)
Layout->>Sidebar: render with inline width (navSize)
Layout->>Wrapper: render with props (panelPosition, showPanel, rightPanelWidth, bottomPanelHeight)
Wrapper->>Content: render main content (flex item)
alt showPanel true
Wrapper->>Panel: render panel (inline width/height based on panelPosition)
else showPanel false
Wrapper-->>Panel: omit or wrap in hidden container (zero-size)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (5)**/*.{js,jsx,ts,tsx,json,md,html,css,scss}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,jsx,json,html,ts,tsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
⏰ 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). (3)
🔇 Additional comments (5)
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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager/components/layout/Layout.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/components/layout/Layout.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/layout/Layout.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/layout/Layout.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/layout/Layout.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/layout/Layout.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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (6)
code/core/src/manager/components/layout/Layout.tsx (6)
174-174: LGTM - Dynamic sidebar width applied correctly.The
navSizeprop is correctly passed toSidebarContainerand applied in the styled component definition below (lines 244-253).
238-238: LGTM - Flex layout correctly applied for desktop.The migration from grid to flex layout is correctly implemented with
flexDirection: 'row'for desktop viewports.
244-253: LGTM - SidebarContainer properly refactored for flex layout.The component correctly accepts and applies the
widthprop, andflexShrink: 0ensures the sidebar maintains its size in the flex layout.
255-266: LGTM - ContentPanelWrapper layout implementation is solid.The flex-based layout with
overflow: hiddenandminWidth: 0correctly addresses the overflow issue described in #30420. The dynamicflexDirectionbased onpanelPositionproperly supports both right and bottom panel layouts.
274-274: LGTM - Minimum content width prevents layout collapse.The
minWidthconstraint ensures the content area doesn't shrink below a usable size (100px), which is a good defensive measure in the flex layout.
209-216: Thehiddenattribute implementation is correct and will prevent focus across supported browsers.The HTML
hiddenattribute is a standard feature specified by WHATWG that prevents element rendering and removes the element from keyboard focus order. Browsers implement this viadisplay:noneorcontent-visibility:hidden, and this behavior is consistently supported across Chrome, Brave, and Safari—no special handling or workarounds required. The React patternhidden={condition || undefined}correctly toggles the attribute presence. The implementation will work as intended for your use case.
|
@Sidnioulz please cross-reference this work with #33326 |
Yes I could repro consistently on |
Sidnioulz
force-pushed
the
sidnioulz/issue-30420
branch
from
0b694f2 to
fee0fe8
Compare
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Sidnioulz
force-pushed
the
sidnioulz/issue-30420
branch
from
fee0fe8 to
542246b
Compare
Closes #30420
What I did
hiddenwhen the addon is 0px wide/tall, to ensure addons cannot be focused intoI keep thinking a better solution would be not to render those addons. But then of course, users would need to wait for addon UIs to reload when reopening the addon panel. We'd need some data to understand if people who hide the panel tend to keep it hidden, or to hide and show it in quick successions.
Checklist for Contributors
Testing
Not easily testable as it only triggers with particularly large sidebars.
Manual testing
Documentation
ø
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
✏️ Tip: You can customize this high-level summary in your review settings.