Conversation
|
|
||
| const getHeaderCheckboxAriaLabel = (): string => { | ||
| const totalColumns = getTotalColumns(); | ||
| return `Select all rows, Column 1 of ${totalColumns}`; |
There was a problem hiding this comment.
no need to read out the row/column info as that's typically done by the screen reader. can cmiiw if that's different on other platforms
| return `Select all rows, Column 1 of ${totalColumns}`; | |
| return `Select all rows`; |
There was a problem hiding this comment.
yeah it is working ok on macbook VO but android and windows wasnt sounding it out
There was a problem hiding this comment.
I'm wondering how useful it is to read the column for a select all checkbox
There was a problem hiding this comment.
can drop it and defer to default screen reader behaviour. it seems alright in Talkback
benjaminLeongSK
force-pushed
the
BL/1073
branch
from
cbcbc8a to
a30ec09
Compare
| typeof label === "string" | ||
| ? isSortable | ||
| ? getSortButtonAriaLabel( | ||
| label, | ||
| fieldKey | ||
| ) | ||
| : label | ||
| : undefined |
There was a problem hiding this comment.
this whole logic could be moved to getSortButtonAriaLabel
also thinking that instead of checking for label string only, you could use aria-labelledby pointing to the header cell wrapper
There was a problem hiding this comment.
not sure if i understood what you meant correctly. but wont aria-labelledby override aria-label? or you meant to use aria-labelledby instead of aria-label something like this?:
<VisuallyHidden>
<button
type="button"
aria-labelledby={
isSortable
? getSortButtonAriaLabel(label, fieldKey)
: wrapperId
}
onClick={() => onHeaderClick?.(fieldKey)}
/>
</VisuallyHidden>
)}
<HeaderCellWrapper id={wrapperId}>
{typeof label === "string" ? (
There was a problem hiding this comment.
<HeaderCellWrapper id="headerCellWrapperId">
{label} // <---------- original code
</HeaderCellWrapper>
<VisuallyHidden>
<button>Sort <span aria-labelledby="headerCellWrapperId" /> by x order</button>
</VisuallyHidden>
| position: relative; | ||
| z-index: 1; |
There was a problem hiding this comment.
do we still need these additional position styles on HeaderCell and HeaderCellWrapper?
There was a problem hiding this comment.
what about position relative btw?
| const headerRef = useRef<HTMLTableSectionElement>(null); | ||
| const actionBarRef = useRef<HTMLDivElement>(null); | ||
| const wrapperRef = useRef<HTMLDivElement>(null); | ||
| const internalId = useRef(id || SimpleIdGenerator.generate()); |
There was a problem hiding this comment.
use state instead so that you don't have to access via .current each time
| return `${internalId.current}-row-${rowId}-position`; | ||
| }; | ||
|
|
||
| const getKeyColumnCellId = (rowId: string, fieldKey: string) => { |
There was a problem hiding this comment.
| const getKeyColumnCellId = (rowId: string, fieldKey: string) => { | |
| const getCellId = (rowId: string, fieldKey: string) => { |
| <VisuallyHidden id={getRowPositionId(rowId)}> | ||
| {`Row ${index + 1} of ${rows?.length ?? 0}`} | ||
| </VisuallyHidden> |
There was a problem hiding this comment.
this too. should be handled by the screen reader
There was a problem hiding this comment.
hmm the screen reader wont read out the rows by default. unless we add it as part of the aria-describedBy?
2 participants
Type of changes
Description of changes
scrollableandkeyFieldLabelpropChecklist
Looks good on mobile and tablet
Updated documentation
Screen reader annotates as per figma
Link to Figma