-
Notifications
You must be signed in to change notification settings - Fork 14
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
508 changes for sds-table expand #1545
Conversation
|
@@ -65,6 +65,7 @@ | |||
*matCellDef="let element" | |||
[attr.colspan]="rowConfig.displayedColumns.length" | |||
class="sds-table__cell--detail" | |||
role="rowgroup" |
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.
Are you sure rowgroup is the right role? I seem to remember this TD is just a maxColumn cell. Looking at MDN it looks like the expectation is that a rowgroup contains other rows of cells, while I'm pretty sure this TD just contains some content, not any rows of other cells.
If you need to put a role on this element, maybe cell makes more sense?
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.
updated to cell
@@ -84,6 +85,11 @@ | |||
*matCellDef="let element" | |||
class="cursor-pointer maxw-3 padding-top-2 padding-right-0 padding-left-1" | |||
(click)="onExpansionClicked(element)" | |||
[attr.aria-expanded]="element == expandedElement ? 'true' : 'false'" | |||
[attr.aria-label]="element == expandedElement ? 'Collapse details' : 'Expand details'" |
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 think it might make sense to have this come from the configuration. I'm sure there is a use-case out there where something other than details are being displayed, and being able to be more specific about what details are being expanded could be useful.
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.
updated the demo to be able to define the labels
Also you probably should add something like |
I edited the description to link the story no. with this PR |
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.
LGTM
fixed 508 issues.
Description
for sds table expand fixed keyboard navigation for expand buttons
Motivation and Context
resolves #1148
Type of Change (Select One and Apply Github Label)
Screenshots (if appropriate):
Which browsers have you tested?
Checklist: