fix: make voicover read title and description out of the table#4288
fix: make voicover read title and description out of the table#4288JamalAlabdullah wants to merge 4 commits into
Conversation
JamalAlabdullah
added
kind/bug
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAppTable extends its props to accept ChangesCaption rendering and table accessibility refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/layout/SigningDocumentList/SigningDocumentListComponent.test.tsx (1)
128-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLoading-state test should also assert the download column header.
The render test validates the new visually-hidden download header, but the loading test still omits that assertion. Adding it keeps accessibility coverage consistent across states.
Suggested test addition
screen.getByRole('columnheader', { name: 'signing_document_list.header_filename' }); screen.getByRole('columnheader', { name: 'signing_document_list.header_attachment_type' }); screen.getByRole('columnheader', { name: 'signing_document_list.header_size' }); + screen.getByRole('columnheader', { name: 'signing_document_list.download' }); screen.getByRole('cell', { name: /loading data.../i });🤖 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/layout/SigningDocumentList/SigningDocumentListComponent.test.tsx` around lines 128 - 133, The loading state test block for SigningDocumentListComponent is missing an assertion for the download column header that is being tested in the render test. Add a screen.getByRole assertion for the columnheader with the download header name (following the same pattern as the existing columnheader assertions for filename, attachment_type, and size) to ensure consistent accessibility coverage across all component states.
🤖 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/layout/SigningDocumentList/SigningDocumentListComponent.tsx`:
- Around line 22-23: The table labeling contract was dropped when the caption
was removed from AppTable in SigningDocumentListComponent.tsx. The tableLabelId
constant is defined but only used for description IDs, leaving the table without
a programmatic label. In src/app-components/Table/Table.tsx, add an
ariaLabelledBy prop to the AppTable component and forward it to the underlying
Table element as aria-labelledby to restore the accessible table naming. Then in
SigningDocumentListComponent.tsx at lines 22-23 (and the other affected
locations at lines 42-66 and 66-115), pass the tableLabelId to each AppTable
usage via the new ariaLabelledBy prop to establish the table labeling
relationship.
---
Outside diff comments:
In `@src/layout/SigningDocumentList/SigningDocumentListComponent.test.tsx`:
- Around line 128-133: The loading state test block for
SigningDocumentListComponent is missing an assertion for the download column
header that is being tested in the render test. Add a screen.getByRole assertion
for the columnheader with the download header name (following the same pattern
as the existing columnheader assertions for filename, attachment_type, and size)
to ensure consistent accessibility coverage across all component states.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ade350aa-9c14-49ea-9ffb-812439860b4f
📒 Files selected for processing (2)
src/layout/SigningDocumentList/SigningDocumentListComponent.test.tsxsrc/layout/SigningDocumentList/SigningDocumentListComponent.tsx
JamalAlabdullah
added
the
squad/utforming
walldenfilippa
left a comment
walldenfilippa
left a comment
There was a problem hiding this comment.
I tested it with NVDA, and the header labels and columns are read correctly 🥳
However, the description is still being read twice. See this comment from Olav in the original issue: #4278 (comment)
| return <SigningDocumentListError error={error} />; | ||
| } | ||
|
|
||
| const downloadLabel = langAsString('signing_document_list.download'); |
There was a problem hiding this comment.
Consider using all labels as variables, or integrating them directly into the code for consistency.
| import { getSizeWithUnit } from 'src/utils/attachmentsUtils'; | ||
| import type { ITextResourceBindings } from 'src/layout/layout'; | ||
|
|
||
| const tableLabelId = 'signing-document-list'; |
There was a problem hiding this comment.
This id is not unique. Is their a usecase where someone would use two SigningDocumentList component?
You could pass the baseComponentId which is available trough the index.tsx, to remove that risk :)
| renderCell: (_, rowData) => ( | ||
| <Link | ||
| href={rowData.url} | ||
| style={{ display: 'flex', gap: '0.5rem', whiteSpace: 'nowrap', textDecoration: 'none' }} |
There was a problem hiding this comment.
Consider moving the styling into a dedicated module.
lassopicasso
left a comment
lassopicasso
left a comment
There was a problem hiding this comment.
Testet denne, tittel og beskrivelse blir lest korrekt.
| <> | ||
| {textResourceBindings?.title && ( | ||
| <div className={captionClasses.tableCaption}> | ||
| <Heading |
There was a problem hiding this comment.
Vi bør ha dynamisk heading etter hvor komponenten ligger i rekkefølge, om den ligger etter en h2 så bør vel neste være h3 om den er etter en h1 så blir det h2 - eller har vi et fast mønster på dette ?
There was a problem hiding this comment.
Alternativt så kan vi gå for å bruke title og description - og ikke heading
|
4 participants
Description
after.mov
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
Summary by CodeRabbit