fix(a11y): add accessible name to AmountInput for screen readers (#427) - #438
Conversation
|
@Amas-01 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@Amas-01 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
collinsezedike
left a comment
There was a problem hiding this comment.
CI is failing on pnpm format:check: prettier --check flags apps/web/src/__tests__/components/AmountInput.test.tsx. Run pnpm prettier --write apps/web/src/__tests__/components/AmountInput.test.tsx and push the result.
Worth noting for next time: format:check is a separate script from pnpm lint, so pnpm lint && pnpm typecheck && pnpm test (what your test plan lists) won't catch this on its own.
Everything else here is solid, thorough PR body, real accessibility assertions via getByRole with a name filter rather than just checking the attribute exists, and the pre-existing unrelated test failure you flagged is appreciated context.
collinsezedike
left a comment
There was a problem hiding this comment.
Approving. The accessibility assertion approach here is solid, using getByRole with a name filter is a real hard check that the accessible name exists, not just that an attribute is present.
Thank you for the contribution. Feel free to grab another open issue.
Merging now.
Summary
apps/web/src/components/ui/AmountInput.tsx: addedaria-labelto the<input>element, derived from the existingcurrencyprop — value isAmount in ${currency}apps/web/src/__tests__/components/AmountInput.test.tsx: created new test file with 5 accessibility and regression tests asserting the input's accessible name includes the currencyApproach taken
aria-labelwas chosen overaria-labelledbybecause:aria-labelledbyor<label htmlFor>wiring — the only ARIA pattern used anywhere is plainaria-labelon iconographic buttons (Toasts.tsx,WalletConnect.tsx)<span>has no stableid, and adding one would require also adding anidprop to the component with no existing convention to followaria-labelderived from the already-availablecurrencyprop requires no new props, no JSX restructuring, and no call site changesAccessible name value
Template:
Amount in ${currency}Examples: "Amount in USDC" (DepositTab), "Amount in mUSDC" (WithdrawTab)
Verification
getByRole('spinbutton', { name: /amount in usdc/i })passes — confirms the accessible name is present and includes the currencygetByRolewith anamefilter throws if no matching accessible name exists, making it a hard accessibility assertion without requiring axeaxe audit: not run —
jest-axe/@axe-core/reactare not installed in this project. Documented in the test file with a comment.Test plan
pnpm lint && pnpm typecheck && pnpm testpass locally (57/57 tests pass; 1 pre-existing unrelated failure inuseVaultActions.test.tsdocumented below)getByRole('spinbutton', { name: /amount in usdc/i })succeeds — accessible name confirmed presentBreaking change
No — no existing prop was changed or removed. No call site was modified.
Additional findings
The
useVaultActions.test.tsfile has a pre-existing failure (Cannot find module '@meridian/shared') present onmainbefore this change. It is caused by the workspace package not being built in the test environment and is unrelated to this PR.Closes #427