(SP: 1) [Frontend] Q&A Per-Page Dropdown UI Polish (Style Consistency + Footer Overlap Fix)#445
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the native page-size Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/components/tests/q&a/pagination.test.tsx (1)
146-148: Add a keyboard-path regression test for the new dropdown.This only verifies the click flow, so the accessibility gap in the custom listbox can slip through. Please cover open/select/close via keyboard here as well, ideally with
userEventso focus behavior is exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/tests/q`&a/pagination.test.tsx around lines 146 - 148, The test currently only exercises the click flow using screen.getByLabelText('itemsPerPageAria') and fireEvent.click(...), which misses keyboard accessibility; update the test to add a keyboard-path regression: use userEvent (imported as userEvent) to focus the trigger (itemsPerPageAria), simulate opening the listbox with keyboard (e.g., Enter or Space), navigate to the "40" option via ArrowDown/ArrowUp or type the option label, activate it with Enter, and then assert the listbox closed and selection applied; replace or augment the fireEvent.click calls that target screen.getByRole('option', { name: '40' }) with userEvent keyboard interactions so focus behavior and keyboard accessibility of the custom listbox are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/components/q`&a/Pagination.tsx:
- Around line 218-239: The hard-coded IDs "qa-page-size-label" and
"qa-page-size-trigger" cause collisions when multiple Pagination components
mount; generate instance-scoped IDs (e.g., with React's useId() or a unique
suffix) and replace the static id attributes on the label and button with those
generated IDs, then update aria-labelledby and any other references
(pageSizeListboxId usage, aria-controls) to use the matching instance-scoped
IDs; ensure the logic around setIsPageSizeOpen, isPageSizeOpen,
pageSizeDropdownRef, and pageSizeListboxId continues to reference the new IDs so
accessibility relationships remain correct.
- Around line 231-289: The page-size dropdown in Pagination.tsx uses
listbox/option roles but lacks keyboard semantics; update the component (using
the qa-page-size-trigger button, isPageSizeOpen/setIsPageSizeOpen,
pageSizeListboxId, pageSizeOptions, onPageSizeChange, pageSize) to implement
proper listbox keyboard interactions: when opening move focus into the listbox
to the selected option (or first option), implement a keydown handler on the
listbox that handles ArrowDown/ArrowUp to move focus (roving tabindex or track
an activeIndex), Home/End to jump to first/last, and Enter/Space to select the
focused option; also keep Escape closing behavior and maintain aria-selected and
aria-activedescendant (or update option tabindex) so screen readers and keyboard
users can navigate correctly, or alternatively replace this custom logic with a
headless/select primitive (e.g., Headless UI/Downshift) that provides the
required semantics.
---
Nitpick comments:
In `@frontend/components/tests/q`&a/pagination.test.tsx:
- Around line 146-148: The test currently only exercises the click flow using
screen.getByLabelText('itemsPerPageAria') and fireEvent.click(...), which misses
keyboard accessibility; update the test to add a keyboard-path regression: use
userEvent (imported as userEvent) to focus the trigger (itemsPerPageAria),
simulate opening the listbox with keyboard (e.g., Enter or Space), navigate to
the "40" option via ArrowDown/ArrowUp or type the option label, activate it with
Enter, and then assert the listbox closed and selection applied; replace or
augment the fireEvent.click calls that target screen.getByRole('option', { name:
'40' }) with userEvent keyboard interactions so focus behavior and keyboard
accessibility of the custom listbox are exercised.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7df192db-8692-4312-9701-554925cd37ce
📒 Files selected for processing (2)
frontend/components/q&a/Pagination.tsxfrontend/components/tests/q&a/pagination.test.tsx
…istbox navigation
Goal
Bring Q&A "Per page" control in line with pagination style and remove footer overlap.
Changes
Closes #444
Summary by CodeRabbit
New Features
Tests