♿️(frontend) fix sidebar resize handle for screen readers#2122
♿️(frontend) fix sidebar resize handle for screen readers#2122
Conversation
Ovgodd
force-pushed
the
fix/separator-keyboard-interact
branch
from
aa9cacf to
c22927b
Compare
Ovgodd
force-pushed
the
fix/separator-keyboard-interact
branch
from
c22927b to
b7d0ba7
Compare
|
Size Change: +2 B (0%) Total Size: 4.27 MB 📦 View Changed
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds localized accessibility for the Impress left-panel resize control. Introduces a stable Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 11-12: The second changelog line ("♿️(frontend) fix sidebar resize
handle for screen readers `#2122`") is missing a list marker and is treated as a
continuation of the previous bullet; add a list marker (e.g., a leading "-" or
"•") before that line so it becomes its own list item under the "Changed"
section, ensuring the two entries ("💄(frontend) improve comments highlights
`#1961`" and "♿️(frontend) fix sidebar resize handle for screen readers `#2122`")
are separate bullets.
In
`@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx`:
- Around line 120-131: The current updateValueText function hardcodes the unit
"pixels" inside the translation key passed to t(...) (see updateValueText,
handle.setAttribute('aria-valuetext')), which prevents translators from
controlling unit formatting or order; change the call to t to pass the numeric
width as a variable only (e.g., t('Sidebar width', { widthPx })) and move the
unit text into the translation string itself (or use ICU/pluralization patterns)
so translators can provide the full localized phrase (including "px" or
localized unit) in the translation files.
- Line 154: Remove the unsupported keyboardResizeBy prop from the PanelGroup
usage in ResizableLeftPanel (the JSX element PanelGroup in
ResizableLeftPanel.tsx); update the JSX to omit keyboardResizeBy entirely and
rely on the library's Separator components for keyboard accessibility per
react-resizable-panels API, ensuring there are no leftover references to
keyboardResizeBy elsewhere in this component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0326f0b8-0ef0-4330-9795-3d271f0727c6
📒 Files selected for processing (2)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx
Ovgodd
force-pushed
the
fix/separator-keyboard-interact
branch
from
a257fa6 to
bfeeba5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx`:
- Around line 172-186: The aria-orientation on the slider is incorrect
(currently "vertical") and must be changed to "horizontal" to match the actual
keyboard interaction and role="slider" behavior; locate the element using
RESIZE_HANDLE_ID in ResizableLeftPanel (the same block that sets
aria-valuemin/aria-valuemax/aria-valuenow/aria-valuetext and references
getSidebarWidthLabel) and update aria-orientation="vertical" to
aria-orientation="horizontal" so it aligns with keyboardResizeBy={1} on
PanelGroup and W3C APG expectations for a horizontal width slider.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41a277df-55a9-41f6-85c2-713da769dd62
📒 Files selected for processing (2)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx`:
- Around line 178-185: The aria-valuetext currently nests t() around
getSidebarWidthLabel which hides full sentences from extraction; replace the
nested translation with a single static translation key per full sentence: map
the result of getSidebarWidthLabel(panelSizePercent, minPanelSizePercent,
maxPanelSizePercent) to a static key (e.g. 'leftPanel.sidebarWidth.narrow' /
'.medium' / '.wide') and call t(key) directly in the aria-valuetext (update
ResizableLeftPanel.tsx where aria-valuetext is set) so each full sentence is a
separate, extractable translation string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7781cf7e-98cf-4285-bcc1-427de244d7d3
📒 Files selected for processing (1)
src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx (1)
178-191: 🧹 Nitpick | 🔵 TrivialSimplify the
aria-valuetextIIFE with a lookup map.The inline IIFE with chained
ifs is harder to scan than a plain object lookup keyed by the label. Functionally equivalent and keeps all three translation keys statically analyzable for extraction.♻️ Proposed refactor
- aria-valuetext={(() => { - const label = getSidebarWidthLabel( - panelSizePercent, - minPanelSizePercent, - maxPanelSizePercent, - ); - if (label === 'narrow') { - return t('Sidebar width: narrow'); - } - if (label === 'medium') { - return t('Sidebar width: medium'); - } - return t('Sidebar width: wide'); - })()} + aria-valuetext={ + { + narrow: t('Sidebar width: narrow'), + medium: t('Sidebar width: medium'), + wide: t('Sidebar width: wide'), + }[ + getSidebarWidthLabel( + panelSizePercent, + minPanelSizePercent, + maxPanelSizePercent, + ) + ] + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx` around lines 178 - 191, Replace the IIFE used for aria-valuetext with a simple lookup map keyed by the result of getSidebarWidthLabel(panelSizePercent, minPanelSizePercent, maxPanelSizePercent); compute const label = getSidebarWidthLabel(...) then map { narrow: t('Sidebar width: narrow'), medium: t('Sidebar width: medium'), wide: t('Sidebar width: wide') }[label] || t('Sidebar width: wide') and use that value for aria-valuetext — this keeps the translation keys statically visible, preserves current behavior, and removes the chained ifs in ResizableLeftPanel.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx`:
- Around line 21-34: getSidebarWidthLabel can produce NaN when max === min
(division by zero) and then incorrectly falls through to 'wide'; update the
function (getSidebarWidthLabel) to guard the degenerate case by checking if (max
=== min || max - min === 0) and return a sensible label (e.g., return 'narrow'
or return current <= min ? 'narrow' : 'wide') before computing ratio, so no
division by zero occurs and screen-reader labels remain correct.
---
Duplicate comments:
In
`@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx`:
- Around line 178-191: Replace the IIFE used for aria-valuetext with a simple
lookup map keyed by the result of getSidebarWidthLabel(panelSizePercent,
minPanelSizePercent, maxPanelSizePercent); compute const label =
getSidebarWidthLabel(...) then map { narrow: t('Sidebar width: narrow'), medium:
t('Sidebar width: medium'), wide: t('Sidebar width: wide') }[label] ||
t('Sidebar width: wide') and use that value for aria-valuetext — this keeps the
translation keys statically visible, preserves current behavior, and removes the
chained ifs in ResizableLeftPanel.tsx.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff32f95c-d995-4f64-a5c3-904caba2abc9
📒 Files selected for processing (1)
src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx
AntoLC
left a comment
AntoLC
left a comment
There was a problem hiding this comment.
Like pointed the rabbit, you should probably managed this case max === min.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx (2)
181-194: 🧹 Nitpick | 🔵 TrivialSimplify the inline IIFE for
aria-valuetext.The three-branch IIFE adds visual noise inside JSX. Since
getSidebarWidthLabelalready returns a discriminated union ('narrow' | 'medium' | 'wide'), a simple object lookup keeps eacht(...)call statically extractable while removing the IIFE wrapper. Alternatively, fold thetcall into a small helper as suggested previously.♻️ Proposed lookup
- aria-valuetext={(() => { - const label = getSidebarWidthLabel( - panelSizePercent, - minPanelSizePercent, - maxPanelSizePercent, - ); - if (label === 'narrow') { - return t('Sidebar width: narrow'); - } - if (label === 'medium') { - return t('Sidebar width: medium'); - } - return t('Sidebar width: wide'); - })()} + aria-valuetext={ + { + narrow: t('Sidebar width: narrow'), + medium: t('Sidebar width: medium'), + wide: t('Sidebar width: wide'), + }[ + getSidebarWidthLabel( + panelSizePercent, + minPanelSizePercent, + maxPanelSizePercent, + ) + ] + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx` around lines 181 - 194, Replace the inline IIFE used for aria-valuetext with a simple lookup using the discriminated union returned by getSidebarWidthLabel: call getSidebarWidthLabel(panelSizePercent, minPanelSizePercent, maxPanelSizePercent) once, then map its 'narrow'|'medium'|'wide' result to the corresponding t('Sidebar width: ...') string (or use a tiny helper that wraps t) and pass that value to aria-valuetext; update the ResizableLeftPanel component where aria-valuetext is set to remove the IIFE and use the lookup/helper instead.
21-37: 🧹 Nitpick | 🔵 TrivialMove the
max <= minguard before computingratio.The guard executes after
ratiois already computed, meaning the function still divides by zero (producingNaN) on every degenerate-range call before bailing out. Functionally the early return saves the day, but readers naturally read the function top-to-bottom and the current order makes the guard look like dead code or an after-thought. Reordering also future-proofs against later edits that move logic between the two.♻️ Proposed reorder
const getSidebarWidthLabel = ( current: number, min: number, max: number, ): 'narrow' | 'medium' | 'wide' => { - const ratio = (current - min) / (max - min); if (max <= min) { return 'medium'; } + const ratio = (current - min) / (max - min); if (ratio < 1 / 3) { return 'narrow'; } if (ratio < 2 / 3) { return 'medium'; } return 'wide'; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx` around lines 21 - 37, The function getSidebarWidthLabel computes ratio before checking the degenerate range, which can produce a divide-by-zero NaN; reorder the guard so the max <= min check occurs before calculating ratio (i.e., return 'medium' immediately if max <= min), then compute ratio = (current - min) / (max - min) and proceed with the existing ratio checks for 'narrow'|'medium'|'wide'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx`:
- Around line 181-194: Replace the inline IIFE used for aria-valuetext with a
simple lookup using the discriminated union returned by getSidebarWidthLabel:
call getSidebarWidthLabel(panelSizePercent, minPanelSizePercent,
maxPanelSizePercent) once, then map its 'narrow'|'medium'|'wide' result to the
corresponding t('Sidebar width: ...') string (or use a tiny helper that wraps t)
and pass that value to aria-valuetext; update the ResizableLeftPanel component
where aria-valuetext is set to remove the IIFE and use the lookup/helper
instead.
- Around line 21-37: The function getSidebarWidthLabel computes ratio before
checking the degenerate range, which can produce a divide-by-zero NaN; reorder
the guard so the max <= min check occurs before calculating ratio (i.e., return
'medium' immediately if max <= min), then compute ratio = (current - min) / (max
- min) and proceed with the existing ratio checks for 'narrow'|'medium'|'wide'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 800f5388-0105-4098-bc66-9caf1fe30bda
📒 Files selected for processing (1)
src/frontend/apps/impress/src/features/left-panel/components/ResizableLeftPanel.tsx
Ovgodd
force-pushed
the
fix/separator-keyboard-interact
branch
from
a2eeb31 to
51e5853
Compare
Expose the handle as a slider so arrow keys work with NVDA
Ovgodd
force-pushed
the
fix/separator-keyboard-interact
branch
from
51e5853 to
b196700
Compare
2 participants
Purpose
Fix left sidebar keyboard resize with a screen reader on, and improve SR output plus step size.
Proposal
aria-valuenowintoaria-valuetext(width in px).keyboardResizeBy={1}for smaller steps.