UI: Make addon panel content scrollable#33858
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughA single-line UI styling change was added to the ControlsPanel component. The AddonWrapper element now includes an Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (1)
code/core/src/components/components/addon-panel/addon-panel.tsx (1)
40-46: Confirm no double-scrollbar regression for addon content that manages its own horizontal overflow.Enabling
horizontalon the outerScrollAreais correct for the stated fix. The only scenario to sanity-check is whether any addon (e.g., a data-grid-style Controls table) already wraps its content in its own horizontal scroll container — the outerScrollArea horizontalwould then produce a nested horizontal scrollbar. Given thatAriaTabPanelintentionally suppresses its own overflow (overflowY: hiddenwhenhasScrollbar=false) to delegate scroll responsibility entirely to thisScrollArea, the design intent is consistent. Manual verification against a few different addons (not just Controls) is recommended.Based on learnings from
AriaTabPanel.tsx:27-30: "the Panel container intentionally maintainsoverflowY: hidden… forcing consumers to implement their own scrolling within tab content rather than relying on automatic browser scrolling, preventing double scrollbars and giving full control over scroll behavior."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/components/components/addon-panel/addon-panel.tsx` around lines 40 - 46, The change enabling horizontal on the outer ScrollArea can cause a nested horizontal scrollbar for addons that already implement their own horizontal scrolling; test relevant addons (e.g., Controls data-grid, tables) by opening them in the panel and verifying there is only one horizontal scrollbar. If you observe double scrollbars, modify addon-panel.tsx to avoid nesting by either conditionally passing horizontal to ScrollArea (only when the child does not manage horizontal overflow) or by forcing the outer container to hide overflowX (e.g., set overflowX: hidden on the ScrollArea wrapper) so the child’s scrollbar is the single control; use the identifiers ScrollArea, useUpdate, hasScrollbar, active, children and remember AriaTabPanel’s overflow behavior when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/components/components/addon-panel/addon-panel.tsx`:
- Around line 40-46: The change enabling horizontal on the outer ScrollArea can
cause a nested horizontal scrollbar for addons that already implement their own
horizontal scrolling; test relevant addons (e.g., Controls data-grid, tables) by
opening them in the panel and verifying there is only one horizontal scrollbar.
If you observe double scrollbars, modify addon-panel.tsx to avoid nesting by
either conditionally passing horizontal to ScrollArea (only when the child does
not manage horizontal overflow) or by forcing the outer container to hide
overflowX (e.g., set overflowX: hidden on the ScrollArea wrapper) so the child’s
scrollbar is the single control; use the identifiers ScrollArea, useUpdate,
hasScrollbar, active, children and remember AriaTabPanel’s overflow behavior
when making this change.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for the PR @Giada-De-Martino!
The AddonPanel component affects many addons, including third-party community addons, and we usually want to avoid two-side scrollable areas because they're less usable.
So instead of changing AddonPanel, could you please look at the Controls addon, remove the AddonPanel dependency it might have, and add a two-way ScrollArea inside that specific addon panel? I think we already do that for the ActionLogger which you could use as an example.
Thanks,
d5ed419 to
31d8cda
Compare
|
Hi @Sidnioulz, |
Sidnioulz
left a comment
There was a problem hiding this comment.
Could you please try using <ScrollArea horizontal vertical> as suggested? Our preferred style for most scrollbars is to have them appear on hover/focus instead of all the time. With overflow: auto, we end up with a very visible scrollbar at the bottom.
I'm also gonna cc @MichaelArestad to get a second design opinion here. Thinking back on this, I think enabling dual-direction scrolling would lead to a WCAG Reflow violation.
@MichaelArestad do you reckon we can adjust layout in the ArgsTable to make it display in a more list-like style when there is too little space, using a container query? This would eliminate the need to scroll horizontally, and also fix the same layout issue in MDX ArgsTables.
@Giada-De-Martino I suggest waiting for Michael to give us confirmation before you resume work, so we don't waste any of your time :)
|
We've talked with Michael and decided to go with a temporary horizontal scrollbar, but using a slightly different architecture. To avoid more back and forth on this small issue, I've opened a PR at #34248. So I'm gonna go ahead and close this one. Looking forward to your next PR! |
Closes #33856
What I did
Wrapped the addon panel content in a ScrollArea component to enable both vertical and horizontal scrolling when the panel is positioned on the right.
File modified: ./code/core/src/components/components/addon-panel/addon-panel.tsx
Change introduced:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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.ts[x ] Make 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.Summary by CodeRabbit