-
Notifications
You must be signed in to change notification settings - Fork 12
fix(data-grid-cell): add button role announcement for screen readers (VIV-2606) #2325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(data-grid-cell): add button role announcement for screen readers (VIV-2606) #2325
Conversation
View Diff1907c1907
< "description": "String that gets applied to the the css gridTemplateColumns attribute of child rows",
---
> "description": "String that gets applied to the css gridTemplateColumns attribute of child rows", |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2325 +/- ##
============================================
Coverage 100.00% 100.00%
============================================
Files 123 383 +260
Lines 1562 17696 +16134
Branches 108 2936 +2828
============================================
+ Hits 1562 17696 +16134
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good !
…nced-without-button-role
<div class="base"> | ||
<div | ||
class="base" | ||
role="${(x) => (shouldShowSortIcons(x) ? 'button' : undefined)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that is what we want, because the the whole header cell becomes a button now
So maybe the role should be just on the sort icon and it should have a label of "Sort"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took my inspiration from these two sources:
- https://www.w3.org/WAI/ARIA/apg/patterns/grid/examples/data-grids/
- https://mulder21c.github.io/aria-practices/examples/table/sortable-table.html
In both of these examples the header cell element got its aria-sort
attributes, and their children are either native buttons, or roles set to button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm, in our data grid you could theoretically have buttons inside the header as well (like filter), which would be inside the button now.
Some data-grids have sort at a separate button like that: https://mui.com/x/react-data-grid/sorting/
But I'm not sure what the best way is, so I'll approve it because at least it's better than before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks :).
Also - I understand your point. Thanks for the example.
I've tried it first, but it felt kinda unnatural for me, to use the tab button to reach the sorting feature, while using roving tabindex for the rest of the table.
I found this solution to be the cleanest, without adding "use tab key to use sorting button" type labels and descriptions and then dealing with i18n.
…nced-without-button-role
No description provided.