UI: Handle kb nav edge cases when preview and panel are hidden #33588
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
View your CI Pipeline Execution ↗ for commit 426ef82
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughRefactors layout by introducing three dedicated components (Drag, PanelContainer, MainAreaContainer), updates Layout.tsx to use them, expands storybook play tests for focus/visibility, and adjusts Sidebar skip-link behavior based on view mode. Changes
Sequence Diagram(s)(Skipped — changes are primarily component extraction and local layout rendering; no multi-actor sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@code/core/src/manager/components/layout/Drag.tsx`:
- Around line 14-28: The drag handle is invisible to keyboard users and has no
focus or keyboard resizing support; update the Drag component styles to include
a :focus-visible rule matching :hover and ensure the rendered handle element
receives tabindex={0} so it can be focused, and then implement keyboard handlers
in the resize logic inside useDragging (or the handler component that uses
useDragging) to listen for ArrowLeft/ArrowRight/ArrowUp/ArrowDown (and
optionally Shift modifiers for larger steps) to call the same resize/move
functions used by pointer events; ensure focus/blur are handled so focus-visible
shows the handle and reuse existing resize function names (e.g., startDrag,
onDrag, endDrag or whatever functions are exported by useDragging.ts) so
keyboard events invoke the same state updates and callbacks as pointer dragging.
In `@code/core/src/manager/components/layout/Layout.stories.tsx`:
- Around line 192-195: Fix the minor typo in the step description string used in
the story test: update the call to step that currently reads "Verify preview
area is rendered" (inside the Layout.stories.tsx story test where step(...) and
canvas.getByTestId('preview') are used) to remove the extra space so it reads
"Verify preview area is rendered".
In `@code/core/src/manager/components/layout/PanelContainer.tsx`:
- Around line 8-14: Rename the mismatched symbols so names are consistent:
either rename the interface PagesContainerProps and internal memo function
PagesContainer to PanelContainerProps and PanelContainer respectively, or rename
the exported component to PagesContainer; update all references (Props type
annotation, the memoized function name, and the export statement) to use the
same identifier (e.g., PanelContainer, PanelContainerProps, panelResizerRef) and
ensure the component export matches the function name to eliminate the naming
inconsistency between PagesContainerProps, PagesContainer, and PanelContainer.
🧹 Nitpick comments (1)
code/core/src/manager/components/layout/MainAreaContainer.tsx (1)
34-37: Redundantrole="main"on a<main>element.
PagesInnerContaineris astyled.mainelement, which already has an implicitrole="main". Explicitly settingrole: 'main'in the landmark props is redundant and could be removed for cleaner code.Suggested fix
const { landmarkProps } = useLandmark( - { 'aria-labelledby': 'main-content-heading', role: 'main' }, + { 'aria-labelledby': 'main-content-heading' }, mainRef );
Comment on lines
+14
to
+28
| ({ theme }) => ({ | ||
| position: 'absolute', | ||
| opacity: 0, | ||
| transition: 'opacity 0.2s ease-in-out', | ||
| zIndex: 100, | ||
|
|
||
| '&:after': { | ||
| content: '""', | ||
| display: 'block', | ||
| backgroundColor: theme.color.secondary, | ||
| }, | ||
|
|
||
| '&:hover': { | ||
| opacity: 1, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n code/core/src/manager/components/layout/Drag.tsxRepository: storybookjs/storybook
Length of output: 2181
🏁 Script executed:
rg -n "resize|panelSize|navSize|keyboard" --type=ts --type=tsx -g '!*.stories.*' code/core/src/manager | head -50Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
rg -n "focus|keydown|keyup|keypress" --type=ts --type=tsx code/core/src/manager/components/layout/ | head -30Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
rg -n "from.*Drag|import.*Drag" --type=ts --type=js code/core/src/manager/components/layout/ -A 2Repository: storybookjs/storybook
Length of output: 1230
🏁 Script executed:
rg -n "<Drag|<Drag " code/core/src/manager/components/layout/ -B 2 -A 2Repository: storybookjs/storybook
Length of output: 1029
🏁 Script executed:
rg -l "onMouseDown|onKeyDown|keyboard.*resize|aria-label.*resize" code/core/src/manager/components/layout/ --type=ts --type=jsRepository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
cat -n code/core/src/manager/components/layout/PanelContainer.tsxRepository: storybookjs/storybook
Length of output: 2575
🏁 Script executed:
cat -n code/core/src/manager/components/layout/Layout.tsx | head -200Repository: storybookjs/storybook
Length of output: 8295
🏁 Script executed:
cat -n code/core/src/manager/components/layout/useDragging.tsRepository: storybookjs/storybook
Length of output: 7738
🏁 Script executed:
rg -n "keyboard|shortcut|hotkey|ArrowLeft|ArrowRight|ArrowUp|ArrowDown" code/core/src/manager/ --type=ts --type=js -g '!*.stories.*' | grep -i "resize\|nav\|panel\|size" | head -20Repository: storybookjs/storybook
Length of output: 2208
🏁 Script executed:
rg -n "role.*slider|role.*separator|tabindex" code/core/src/manager/components/layout/ --type=ts --type=jsRepository: storybookjs/storybook
Length of output: 47
Drag handle is keyboard-inaccessible and lacks focus indicators.
The drag handle has opacity: 0 by default and only reveals itself on mouse hover with no keyboard equivalent. Keyboard users cannot resize panels as there are no keyboard event handlers in useDragging.ts and no keyboard shortcuts for resizing anywhere in the codebase. To make this accessible:
- Add
:focus-visiblestyle similar to:hoverfor keyboard focus indication - Add
tabindex={0}to make the element focusable - Implement keyboard event handlers (arrow keys) in the resize logic to support keyboard-based resizing
Alternatively, if keyboard resizing is not intended, consider providing an accessible alternative mechanism (e.g., input fields) for users who cannot operate mouse-based drag handles.
🤖 Prompt for AI Agents
In `@code/core/src/manager/components/layout/Drag.tsx` around lines 14 - 28, The
drag handle is invisible to keyboard users and has no focus or keyboard resizing
support; update the Drag component styles to include a :focus-visible rule
matching :hover and ensure the rendered handle element receives tabindex={0} so
it can be focused, and then implement keyboard handlers in the resize logic
inside useDragging (or the handler component that uses useDragging) to listen
for ArrowLeft/ArrowRight/ArrowUp/ArrowDown (and optionally Shift modifiers for
larger steps) to call the same resize/move functions used by pointer events;
ensure focus/blur are handled so focus-visible shows the handle and reuse
existing resize function names (e.g., startDrag, onDrag, endDrag or whatever
functions are exported by useDragging.ts) so keyboard events invoke the same
state updates and callbacks as pointer dragging.
Comment on lines
+192
to
+195
| await step('Verify preview area is rendered', async () => { | ||
| const preview = canvas.getByTestId('preview'); | ||
| expect(preview).toBeInTheDocument(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Minor typo in step description.
There's a double space in "Verify preview area is rendered".
Suggested fix
- await step('Verify preview area is rendered', async () => {
+ await step('Verify preview area is rendered', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await step('Verify preview area is rendered', async () => { | |
| const preview = canvas.getByTestId('preview'); | |
| expect(preview).toBeInTheDocument(); | |
| }); | |
| await step('Verify preview area is rendered', async () => { | |
| const preview = canvas.getByTestId('preview'); | |
| expect(preview).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
In `@code/core/src/manager/components/layout/Layout.stories.tsx` around lines 192
- 195, Fix the minor typo in the step description string used in the story test:
update the call to step that currently reads "Verify preview area is rendered"
(inside the Layout.stories.tsx story test where step(...) and
canvas.getByTestId('preview') are used) to remove the extra space so it reads
"Verify preview area is rendered".
Sidnioulz
force-pushed
the
sidnioulz/panel-overflow-followup
branch
from
ce03b3a to
d752e98
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 192 | 192 | 0 |
| Self size | 75 KB | 75 KB | 🎉 -54 B 🎉 |
| Dependency size | 32.24 MB | 32.25 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.30 MB | 20.32 MB | 🚨 +23 KB 🚨 |
| Dependency size | 16.52 MB | 16.52 MB | 🎉 -4 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 15 KB | 15 KB | 🚨 +12 B 🚨 |
| Dependency size | 28.96 MB | 28.97 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 538 | 538 | 0 |
| Self size | 646 KB | 646 KB | 🎉 -10 B 🎉 |
| Dependency size | 59.22 MB | 59.73 MB | 🚨 +505 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 127 | 127 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🚨 +14 B 🚨 |
| Dependency size | 21.82 MB | 22.32 MB | 🚨 +496 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 159 | 159 | 0 |
| Self size | 30 KB | 30 KB | 🚨 +8 B 🚨 |
| Dependency size | 23.00 MB | 23.61 MB | 🚨 +610 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 117 | 117 | 0 |
| Self size | 35 KB | 35 KB | 🎉 -8 B 🎉 |
| Dependency size | 19.62 MB | 20.11 MB | 🚨 +496 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 278 | 278 | 0 |
| Self size | 24 KB | 24 KB | 🚨 +2 B 🚨 |
| Dependency size | 44.13 MB | 44.64 MB | 🚨 +505 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 16 KB | 16 KB | 🎉 -22 B 🎉 |
| Dependency size | 33.49 MB | 33.50 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 775 KB | 775 KB | 🚨 +219 B 🚨 |
| Dependency size | 67.38 MB | 67.48 MB | 🚨 +94 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 30 KB | 30 KB | 🎉 -40 B 🎉 |
| Dependency size | 65.95 MB | 66.05 MB | 🚨 +94 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1000 KB | 1000 KB | 🎉 -8 B 🎉 |
| Dependency size | 36.82 MB | 36.84 MB | 🚨 +23 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 170 | 170 | 0 |
| Self size | 18 KB | 18 KB | 🚨 +18 B 🚨 |
| Dependency size | 31.26 MB | 31.28 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 57 | 57 | 0 |
| Self size | 732 KB | 1.23 MB | 🚨 +494 KB 🚨 |
| Dependency size | 12.94 MB | 12.94 MB | 🚨 +2 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Not urgent, low priority.
What I did
Follow up on #33450 + improvements on the settings pages keyboard navigation
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.