UI: Fix global shortcuts not showing region focus indicator#34201
Conversation
|
View your CI Pipeline Execution ↗ for commit 70bb0c9
☁️ Nx Cloud last updated this comment at |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors focus-control: replaces landmark-animation callbacks with explicit boolean Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Shortcuts
participant ManagerAPI
participant UIElement
User->>Shortcuts: toggle addon panel / nav
Shortcuts->>ManagerAPI: api.togglePanel() / api.toggleNav()
ManagerAPI-->>Shortcuts: toggled (wasShown flag)
Shortcuts->>ManagerAPI: focusOnUIElement(target, {forceFocus?, poll: true})
ManagerAPI->>UIElement: attempt focus
alt focus succeeded
UIElement-->>ManagerAPI: focused
else focus failed (timeout)
ManagerAPI->>UIElement: focus(document.body) fallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/menu.tsx (1)
23-25: Remove stale commented animation code.The commented legacy block is no longer part of the flow and can be dropped to keep this mapper easier to read.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/preview/tools/menu.tsx` around lines 23 - 25, Remove the stale commented animation block in menu.tsx: delete the three-line commented code that references animateLandmark and document.getElementById(focusableUIElements.sidebarRegion) so the mapper is cleaner and no legacy animation code remains (search for animateLandmark and focusableUIElements.sidebarRegion to locate the commented block).
🤖 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/manager/components/preview/tools/menu.tsx`:
- Around line 23-25: Remove the stale commented animation block in menu.tsx:
delete the three-line commented code that references animateLandmark and
document.getElementById(focusableUIElements.sidebarRegion) so the mapper is
cleaner and no legacy animation code remains (search for animateLandmark and
focusableUIElements.sidebarRegion to locate the commented block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd4dfb28-0936-4b6d-8879-d80f568a96a6
📒 Files selected for processing (4)
code/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/panel/Panel.tsxcode/core/src/manager/components/preview/tools/addons.tsxcode/core/src/manager/components/preview/tools/menu.tsx
There was a problem hiding this comment.
Pull request overview
Fixes missing region focus indication when toggling the sidebar/addon panel via global keyboard shortcuts by ensuring focus is moved to the appropriate landmark/region elements.
Changes:
- Update preview toolbar “show sidebar/panel” actions to conditionally force focus based on input modality.
- Align addon panel root element ID with
focusableUIElements.storyPanelRoot. - Add focus targeting to the global shortcuts handlers when opening the sidebar/panel.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
code/core/src/manager/components/preview/tools/menu.tsx |
Adjusts sidebar toggle to optionally force-focus the sidebar region for keyboard activation. |
code/core/src/manager/components/preview/tools/addons.tsx |
Adjusts addon panel toggle to optionally force-focus the panel region for keyboard activation. |
code/core/src/manager/components/panel/Panel.tsx |
Uses the shared focusableUIElements.storyPanelRoot constant for the panel root ID. |
code/core/src/manager-api/modules/shortcuts.ts |
Ensures opening sidebar/panel via shortcuts focuses the corresponding region to surface the focus indicator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What I did
Follow up on issue found in 10.3 QA: the focus indicator for sidebars and addon panels showed when pressing their respective "show sidebar/panel" buttons but not when using the global keyboard shortcuts.
Checklist for Contributors
Manual testing
🦋 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