fix(a11y): always give DrawerContent an accessible DialogTitle#2451
Open
innolope-dev wants to merge 3 commits into
Open
fix(a11y): always give DrawerContent an accessible DialogTitle#2451innolope-dev wants to merge 3 commits into
innolope-dev wants to merge 3 commits into
Conversation
Radix Dialog (via vaul) logs 'DialogContent requires a DialogTitle' for screen-reader accessibility when a drawer opens without a title. The badge unlock drawer on /home rendered no DrawerTitle, and captureConsole forwarded the warning to Sentry (PEANUT-UI-MJS, ~1.7k events / 854 users in 14 days). Add an accessibleTitle prop to DrawerContent that renders a visually hidden DrawerTitle, use it in BadgeStatusDrawer, and cover the labelling contract with tests.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthrough
ChangesDrawer accessibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code-analysis diffPainscore total: 6208.86 → 6209.32 (+0.46) |
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![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: 1
🧹 Nitpick comments (1)
src/components/Badges/BadgeStatusDrawer.tsx (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the badge-level accessible name.
The shared drawer tests verify
accessibleTitle, but add aBadgeStatusDrawertest that opens the drawer and asserts the dialog name isBadge unlocked: ${displayName}. This protects the caller’s interpolation and wiring from future regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Badges/BadgeStatusDrawer.tsx` at line 40, Add a regression test for BadgeStatusDrawer that opens the drawer with a known displayName and asserts the dialog’s accessible name is interpolated as “Badge unlocked: ${displayName}”. Exercise the caller wiring rather than relying only on shared Drawer tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Global/Drawer/__tests__/Drawer.test.tsx`:
- Around line 50-64: Update the test case “does not trigger the Radix
missing-DialogTitle warning when accessibleTitle is set” so the console.error
spy is always restored, including when rendering or assertions fail. Wrap the
render and assertions in a try/finally that calls errorSpy.mockRestore(), or use
an equivalent afterEach cleanup.
---
Nitpick comments:
In `@src/components/Badges/BadgeStatusDrawer.tsx`:
- Line 40: Add a regression test for BadgeStatusDrawer that opens the drawer
with a known displayName and asserts the dialog’s accessible name is
interpolated as “Badge unlocked: ${displayName}”. Exercise the caller wiring
rather than relying only on shared Drawer tests.
🪄 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: 2513920d-8b3a-4cbe-91b8-3fb75af9bcb0
📒 Files selected for processing (3)
src/components/Badges/BadgeStatusDrawer.tsxsrc/components/Global/Drawer/__tests__/Drawer.test.tsxsrc/components/Global/Drawer/index.tsx
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
Sentry issue PEANUT-UI-MJS:
DialogContent requires a DialogTitle for the component to be accessible for screen reader users.— 1,703 events in 14 days across 854 users, culprit page/home.This is Radix Dialog's dev-mode accessibility warning (our
Drawerwrapper is built on vaul, which is built on Radix Dialog). AnyDrawerContentrendered without aDrawerTitlelogs it to the console on every open, and thecaptureConsoleSentry integration amplifies it into an event per occurrence. It is also a real accessibility defect: screen readers announce the dialog with no name.The one instance currently missing a title (
BadgeStatusDrawer, opened from the badge strip on/home) is fixed in #2448 with a visibleDrawerTitle(the badge name), which is the better a11y outcome there. This PR ships the reusable mechanism and the guardrail so the class of bug can't quietly come back; it no longer touchesBadgeStatusDrawerand shares no files with #2448 — the two PRs can merge in any order.Fix
DrawerContent(src/components/Global/Drawer) now accepts anaccessibleTitleprop that renders a visually hidden (sr-only)DrawerTitle, for drawers whose design has no visible heading — same idiom already used manually inKycStatusDrawer,TransactionDetailsDrawer, andTokenSelector, now available as a one-prop path.accessibleTitleproduces a DialogTitle wired to the dialog'saria-labelledby, an explicitDrawerTitlechild still labels the dialog, and the Radix missing-DialogTitle warning does not fire when the prop is used.Audited all
DrawerContentusages (ContributorsDrawer, CardUnlockDrawer, KycStatusDrawer, TransactionDetailsDrawer, ChooseNetworkDrawer, TokenSelector, QRBottomDrawer, dev DS pages): with #2448, all render aDrawerTitle. The headlessui-basedModalis unaffected (different library, no Radix warning).Impact
Testing
src/components/Global/Drawer/__tests__/Drawer.test.tsx: assertsaccessibleTitlerenders ansr-onlytitle wired to the dialog'saria-labelledby, that an explicitDrawerTitlechild still labels the dialog, and that the Radix missing-DialogTitle console warning does not fire — 3/3 pass (re-verified after scoping out the BadgeStatusDrawer change; the suite is Drawer-generic).pnpm tsc --noEmitclean (0 errors); prettier clean on touched files.