-
Notifications
You must be signed in to change notification settings - Fork 72
[terra-table] Add row selection to table component #1857
Conversation
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.
Update on the test issue: it looks like is being addressed in this PR: https://github.com/cerner/terra-framework/pull/1855/files#diff-038f1481da2f8f54dfaea6e6dd47bd1e2dfc0d58823c972fde3900f1663dc71f |
packages/terra-table/CHANGELOG.md
Outdated
@@ -6,6 +6,7 @@ | |||
|
|||
* Added | |||
* Added the ability to toggle zebra striping for table rows. | |||
* Added row selection mode to the table component. |
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.
This might have been due to a merge/rebase, but this should go under the Unreleased section
@@ -342,14 +413,16 @@ function Table(props) { | |||
hasRowSelection={hasSelectableRows} | |||
displayedColumns={displayedColumns} | |||
rowHeaderIndex={rowHeaderIndex} | |||
onCellSelect={isGridContext ? handleCellSelection : undefined} | |||
onCellSelect={isGridContext || hasSelectableRows ? handleCellSelection : 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 might be missing why this condition was added here. Was this added as part of allowing the row selection cell checkbox to be selectable?
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.
Previously the onCellSelect, which lets us know that a cell was "selected", would only trigger when in the grid context. If we are in row selection mode, hasSelectableRows prop equal to true, then we need to also handle the cell selection.
], | ||
}; | ||
|
||
const RowSelection = () => { |
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 would have expected a test page similar to this one and some accompanying WDIO tests for row selection.
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 agree with you. I covered in jest, but should also have added the appropriate WDIO tests. Thank you for catching this.
@cm9361 I did a functional review as well as tested keyboard and screenreader usage. I thought it looked good, great job! Mouse: Able to select check boxes as expected. Noticed the row color change on mouse hover, nice touch as that will address one of the MPages gaps identified! Keyboard: Able to navigate the checkboxes via tab and select/unselect them via Space. Rows selected/unselected as expected. Screenreader: Tested with Edge and JAWS 2022.2202.38 |
These changes overlaps with some recent merges in main (mainly CHANGELOGs and translations). Can you update this with main and make sure those files are merged correctly? |
@@ -5,6 +5,7 @@ | |||
## 1.43.0 - (October 25, 2023) | |||
|
|||
* Added | |||
* Added examples and test to `terra-table` for row selection mode. |
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.
Can you move this to the released section?
Also looks like these entries for packages/terra-framework-docs/CHANGELOG.md
also got merged in the wrong section:
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.
Noticed in this review that the elements being focused in the selectable table headers, for Table specifically, are the header cells rather than the buttons inside of them. Logged this jira to remedy.
This one looks good for the story though.
Summary
What was changed:
The Terra Table was updated to allow consumers to enable the row selection mode and its corresponding capabilities.
Why it was changed:
Although an accessible table component would not normally have row selection, the capability is required in the Terra Table component for passivity and to allow integration in our clinical workflows.
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
N/A
This PR resolves:
UXPLATFORM-9766
Thank you for contributing to Terra.
@cerner/terra