Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-table] Add row selection to table component #1857

Merged
merged 12 commits into from
Oct 27, 2023
Merged

Conversation

cm9361
Copy link
Contributor

@cm9361 cm9361 commented Oct 24, 2023

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:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

N/A

This PR resolves:

UXPLATFORM-9766


Thank you for contributing to Terra.
@cerner/terra

@cm9361 cm9361 marked this pull request as ready for review October 24, 2023 22:45
@cm9361 cm9361 requested review from sdadn, eawww and kenk2 October 25, 2023 17:02
Copy link
Contributor

@adoroshk adoroshk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (attaching a screenshot). Looks good to me. As the jest failure looks like not related to current code change and is coming from the terra-menu component, approving the change.
Screenshot 2023-10-25 at 1 22 21 PM

@adoroshk
Copy link
Contributor

adoroshk commented Oct 25, 2023

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

@@ -6,6 +6,7 @@

* Added
* Added the ability to toggle zebra striping for table rows.
* Added row selection mode to the table component.
Copy link
Contributor

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}
Copy link
Contributor

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?

Copy link
Contributor Author

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 = () => {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@chrismichalewicz
Copy link

@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
Row selection mode check box was communicated accurately along with instructions to use that worked.
When individual row check boxes had focus Row header was communicated along with check box.
Selecting the check box via space bar effectively communicated that the row was selecting or unselected.

@sdadn
Copy link
Contributor

sdadn commented Oct 26, 2023

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.
Copy link
Contributor

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:

#1858

@github-actions github-actions bot temporarily deployed to preview-pr-1857 October 26, 2023 23:49 Destroyed
Copy link
Contributor

@eawww eawww left a 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.

@cm9361 cm9361 merged commit 4346725 into main Oct 27, 2023
21 checks passed
@cm9361 cm9361 deleted the table-row-selection branch October 27, 2023 16:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants