Migrate table #1834

kenk2 · 2023-10-13T19:40:39Z

Summary

What was changed:
Migrate New Terra Table to Framework from feature branch in Terra Core.

Why it was changed:
We want the Terra Table package to live inside Terra Framework as it will be consumed by Data Grid.

Testing

This change was tested using:

Reviews

In addition to engineering reviews, this PR needs:

UX review
Accessibility review
Functional review

Additional Details

This PR resolves:

UXPLATFORM-9540

Thank you for contributing to Terra.
@cerner/terra

cm9361 · 2023-10-14T23:42:35Z

packages/terra-table/package.json

+    "keycode-js": "^3.1.0",
+    "prop-types": "^15.5.8",
+    "resize-observer-polyfill": "^1.4.1",
+    "terra-content-container": "^3.39.1",


We are not using a Terra ContentContainer component in the Terra Table project that I can see.

cm9361 · 2023-10-14T23:45:44Z

packages/terra-table/package.json

+    "resize-observer-polyfill": "^1.4.1",
+    "terra-content-container": "^3.39.1",
+    "terra-icon": "^3.54.0",
+    "terra-mixins": "^1.40.0",


Are we using terra-mixins in this project? If not, it should not be a dependency.

cm9361 · 2023-10-14T23:47:47Z

packages/terra-table/package.json

+  "name": "terra-table",
+  "main": "lib/index.js",
+  "version": "4.36.0",
+  "description": "The terra-table component provides user a way to render selectable data in a tabular format",


I don't think that the description should mention selectable data since this is the Table component. That would be the purpose of a grid not a table.

Updated the description.

cm9361 · 2023-10-14T23:48:09Z

packages/terra-table/package.json

+{
+  "name": "terra-table",
+  "main": "lib/index.js",
+  "version": "4.36.0",


I thought we were doing a major version bump with this update.

This will be automatically updated once we do the release.

cm9361 · 2023-10-14T23:48:33Z

packages/terra-table/package.json

+  "name": "terra-table",
+  "main": "lib/index.js",
+  "version": "4.36.0",
+  "description": "The terra-table component provides user a way to render selectable data in a tabular format",


Should this be "The Terra Table component ..."

Updated the description.

cm9361 · 2023-10-14T23:49:52Z

packages/terra-table/package.json

+    "Table",
+    "UI"
+  ],
+  "author": "Cerner Corporation",


Should this be Oracle instead of Cerner?

We haven't been given guidance on changing this but if this needs to be changed, we can probably go ahead do it and and update the rest of the packages in a separate PR.

cm9361 · 2023-10-14T23:52:55Z

packages/terra-table/package.json

+    "react-intl": "2 || 3 || 4 || 5"
+  },
+  "dependencies": {
+    "@cerner/terra-docs": "^1.9.0",


I don't believe that terra-docs should be a dependency. At most it is a devDependency, if needed.

I don't think it should be needed at all.

cm9361 · 2023-10-14T23:53:52Z

packages/terra-table/package.json

+    "terra-theme-context": "^1.0.0",
+    "terra-visually-hidden-text": "^2.36.0"


Should our Terra components be peer and dev dependencies?

I think for now since this is used directly in Cell.jsx (until we remove the cell dive logic) that we should probably keep this as a dependency. Once the Cell Dive logic is removed we could probably remove this package altogether.

cm9361 · 2023-10-14T23:56:02Z

packages/terra-table/CHANGELOG.md

Is this the correct changelog? We won't have this release history in this repository, correct?

It will still point to the same package on NPM, so the version history is still valid for the doc site and npm page.

cm9361 · 2023-10-14T23:56:42Z

packages/terra-table/NOTICE

@@ -0,0 +1,13 @@
+Copyright 2017 - 2019 Cerner Innovation, Inc.


Is this correct now that we are Oracle?

We should update the year at least.

cm9361 · 2023-10-15T18:25:14Z

packages/terra-table/package.json

You can use depcheck to find and remove unused dependencies.

cm9361 · 2023-10-15T18:31:48Z

packages/terra-framework-docs/src/terra-dev-site/doc/table/Examples/SortableTable.3.doc.mdx

+
+## Table Props
+
+<TablePropsTable />


Your examples should not have the property table for the Terra Table or its dependencies. That would be part of the main About document.

Removed from all examples.

cm9361 · 2023-10-15T18:35:50Z

You should have a separate example for the Default Terra Table based on similar component strategies.

cm9361 · 2023-10-16T13:54:11Z

packages/terra-table/tests/wdio/table-spec.js

+    it('Validates the default table is not interactable', () => {
+      browser.url('/raw/tests/cerner-terra-framework-docs/table/no-interaction-table');
+      browser.keys(['Tab']);
+      $('#focused-button').isFocused();


You should be doing an expect on this, right? This code does not actually validate that this button is focused.

…r default table. Remove obsolete packages. Fix wdio test.

chrismichalewicz · 2023-10-16T20:01:29Z

@kenk2 It was hard to fully verify this for accessibility and functionally based on how early in the development of Table we are. Functionally with what was there looked good, my only issue was not being able to scroll the examples via keyboard that as you mentioned that is addressed via UXPLATFORM-9729.

I feel like it's too early to fully perform accessibility test so I went ahead removed those labels. I did try testing it with Jaws. One issue I will call out that I encountered is the values of each cell are being communicated as 1 value rather than 3 values. For example, the Default Table example the Heart Rate Monitor row has values 68, 66, and 67 the screen reader will read it as 686,667. If you think this is unexpected at this point and we can explore how best to account for this, whether it's a part of this story or I create a new story specifically to get the AT stuff ironed out.

kenk2 · 2023-10-16T20:22:34Z

@chrismichalewicz I think for now given how early everything is, we can create another story to get the screen reader functionality up to par.

cm9361 · 2023-10-16T20:53:22Z

@chrismichalewicz I think for now given how early everything is, we can create another story to get the screen reader functionality up to par.

I would not expect the screen reader to be off in the initial release. We don't have this issue in the DataGrid, right?

chrismichalewicz · 2023-10-17T15:41:21Z

@kenk2 @cm9361 I retested this morning with Jaws in Edge and am not able to recreate what I was seeing yesterday. Each individual cell value is being read individually now as I would expect. This is working the same in Data Grid as well.

eawww · 2023-10-17T17:46:21Z

packages/terra-framework-docs/src/terra-dev-site/doc/table/About.1.doc.mdx

+
+# Terra Table
+
+Terra Table is a structural component used to create data tables. Table provides means to handle row selection and hooks for sortable columns.


Suggested change

Terra Table is a structural component used to create data tables. Table provides means to handle row selection and hooks for sortable columns.

Terra Table is a structural component used to display tabular data. Table provides means to handle row selection and hooks for sortable columns.

eawww · 2023-10-17T17:52:54Z

packages/terra-framework-docs/src/terra-dev-site/doc/table/Examples/DefaultTable.jsx

+        { content: '' },
+        { content: 'Quinzell, Harleen' },
+        { content: '' },
+        { isMasked: true },


Table examples shouldn't include masked cells. That feature should only be advertised for Worklist Data Grid unless a need for masking in ordinary tables is established.

eawww · 2023-10-17T17:54:28Z

packages/terra-framework-docs/src/terra-dev-site/doc/table/Examples/PinnedColumnsTable.jsx

+    { id: 'Column-7', displayName: 'Patient Age' },
+    { id: 'Column-8', displayName: 'Medication History' },
+    { id: 'Column-9', displayName: 'My Relationship' },
+    { id: 'Column-10', displayName: 'Not Selectable' },


Selectable/Not selectable isn't a feature of table and shouldn't be in the examples. I know this data is likely to change anyway to reflect typical use for a table, but better to catch this early.

eawww · 2023-10-17T17:58:01Z

packages/terra-table/src/subcomponents/Cell.jsx

+  isSelectable: false,
+  isMasked: false,


Why would consumers not be able to mask a cell for a Terra Table? That should not break any accessibility, right?

eawww · 2023-10-17T17:58:37Z

packages/terra-table/src/subcomponents/Cell.jsx

+  /**
+   *  Boolean indicating if cell contents are masked.
+   */
+  isMasked: PropTypes.bool,
+
+  /**
+   * Boolean value indicating whether or not the column header is selectable.
+   */
+  isSelectable: PropTypes.bool,
+
+  /**
+   * Boolean indicating whether the Cell is currently selected.
+   */
+  isSelected: PropTypes.bool,


We need this prop for grid support. The subcomponents are reused.

eawww · 2023-10-17T17:59:09Z

packages/terra-table/src/proptypes/cellShape.js

+  isMasked: PropTypes.bool,
+  /**
+   * Boolean value indicating whether or not the column header is selectable.
+   */
+  isSelectable: PropTypes.bool,
+  /**
+   * Boolean value indicating whether or not the cell should render as selected.
+   */
+  isSelected: PropTypes.bool,


This will be needed because it is reused by the grid components. However, we do not have to publicize in the Table API documentation.

eawww · 2023-10-17T18:30:08Z

@cm9361 I've advised @kenk2 that mentions of API features like Cell Selection and Masking shouldn't appear in the documentation for Terra Table, even if they're needed for components that consume it. That might include making props private.

cm9361 · 2023-10-17T19:21:38Z

@cm9361 I've advised @kenk2 that mentions of API features like Cell Selection and Masking shouldn't appear in the documentation for Terra Table, even if they're needed for components that consume it. That might include making props private.

I agree with this statement. "onCellSelect" should not be part of the "public API". I am not certain removing masked cells is necessary though. If a team desires to use it for some reason, it would still be accessible.

…table examples. Private grid-only props in cell shape.

kenk2 added 2 commits October 13, 2023 12:29

Initial transfer of package from terra-core to terra-framework.

38f1158

Remove all references to terra-core in table package.

adeb159

kenk2 self-assigned this Oct 13, 2023

kenk2 added the 📦 terra-table label Oct 13, 2023

github-actions bot temporarily deployed to preview-pr-1834 October 13, 2023 19:46 Destroyed

Alaias legacy terra table used in test.

016345d

github-actions bot temporarily deployed to preview-pr-1834 October 13, 2023 19:47 Destroyed

Fix failing WDIO tests from bad file paths.

9d3e189

github-actions bot temporarily deployed to preview-pr-1834 October 13, 2023 21:09 Destroyed

kenk2 added 2 commits October 13, 2023 14:25

Added changelog note for terra-framework-docs.

99bef6b

Resolve merge conflicts.

32c5465

github-actions bot temporarily deployed to preview-pr-1834 October 13, 2023 21:29 Destroyed

kenk2 marked this pull request as ready for review October 13, 2023 21:56

kenk2 requested a review from a team October 13, 2023 21:56

cm9361 reviewed Oct 14, 2023

View reviewed changes

cm9361 reviewed Oct 15, 2023

View reviewed changes

cm9361 reviewed Oct 16, 2023

View reviewed changes

Remove props and usage guide from examples. Create seprate example fo…

40d724b

…r default table. Remove obsolete packages. Fix wdio test.

github-actions bot temporarily deployed to preview-pr-1834 October 16, 2023 15:00 Destroyed

kenk2 added Accessibility Review Required UX Review Required labels Oct 16, 2023

Use useCallback in Sortable Table example.

aca12f5

github-actions bot temporarily deployed to preview-pr-1834 October 16, 2023 15:32 Destroyed

kenk2 requested a review from cm9361 October 16, 2023 15:44

cm9361 approved these changes Oct 16, 2023

View reviewed changes

sdadn approved these changes Oct 16, 2023

View reviewed changes

chrismichalewicz added ⭐ Accessibility Reviewed ⭐ Functionally Reviewed and removed Accessibility Review Required Functional Review Required ⭐ Accessibility Reviewed labels Oct 16, 2023

Resolve merge conflicts.

2429408

github-actions bot temporarily deployed to preview-pr-1834 October 17, 2023 15:46 Destroyed

eawww reviewed Oct 17, 2023

View reviewed changes

Private cell selection callback. Remove unselectable cell/columns in …

8a24afa

…table examples. Private grid-only props in cell shape.

github-actions bot temporarily deployed to preview-pr-1834 October 17, 2023 20:36 Destroyed

github-actions bot temporarily deployed to preview-pr-1834 October 17, 2023 20:46 Destroyed

kenk2 merged commit 8b84401 into main Oct 17, 2023
21 checks passed

kenk2 deleted the migrate-table branch October 17, 2023 21:03

sdadn added ⭐ UX Reviewed UX Reviewed and approved. Major Version Bump This issue requires a major version bump to the associated packages and removed UX Review Required labels Oct 17, 2023

sdadn mentioned this pull request Oct 18, 2023

Delete terra-table package cerner/terra-core#3947

Merged

8 tasks

		"terra-theme-context": "^1.0.0",
		"terra-visually-hidden-text": "^2.36.0"

		@@ -0,0 +1,13 @@
		Copyright 2017 - 2019 Cerner Innovation, Inc.


		# Terra Table

		Terra Table is a structural component used to create data tables. Table provides means to handle row selection and hooks for sortable columns.

Migrate table #1834

Migrate table #1834

Conversation

kenk2 commented Oct 13, 2023 • edited Loading

Summary

Testing

Reviews

Additional Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenk2 Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cm9361 commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismichalewicz commented Oct 16, 2023

kenk2 commented Oct 16, 2023

cm9361 commented Oct 16, 2023

chrismichalewicz commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eawww commented Oct 17, 2023

cm9361 commented Oct 17, 2023

kenk2 commented Oct 13, 2023 •

edited

Loading

kenk2 Oct 16, 2023 •

edited

Loading