[terra-table] Adding subsections to sections #2112

BenBoersma · 2024-03-29T21:13:48Z

Summary

Adding subsections to sections which include a section header.

What was changed:

Why it was changed:

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:
Partially resolves.
UXPLATFORM-10224

Thank you for contributing to Terra.
@cerner/terra

…ed an aria-rowindex error in table

adoroshk

Some CSS observations

Lowlight theme styling looks off to me:

2. NIT (if that is expect behavior - please disregard it), subsection header text alignment is not exactly aligned with section header (or anything else) on the left. It is placed much more to the right than the rest of the content.

adoroshk

To make sure we are on the same page I wanted to double-check that subsections are going to be supported on table only in not supported in Grids. If that's correct, that should be also mentioned in documentation. In case I am wrong and subsections are supported in grids, there should be grid examples and tests added.

adoroshk · 2024-04-02T15:00:08Z

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

@@ -0,0 +1,90 @@
+// TODO: fix linter error


Is this TODO addressed, or is it out of scope of this PR?

Looks like it was from a previous PR but there doesn't seem to be any linter issues. Removed here: 86bd928

adoroshk · 2024-04-02T15:21:33Z

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

+  /**
+   * An array of subsections objects to be rendered within the section.
+   */
+  subsections: PropTypes.arrayOf(subsectionShape),


adoroshk · 2024-04-02T15:23:24Z

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

+import PropTypes from 'prop-types';
+import rowShape from './rowShape';
+
+const sectionShape = PropTypes.shape({


A documentation page needs to be updated with section shape info similar to the way we have for section

Added here: 2395c42

adoroshk · 2024-04-02T15:27:37Z

...ework-docs/src/terra-dev-site/test/table/TableWithCollapsibleSectionsAndSubSections.test.jsx

+  };
+
+  return (
+    <Table


I would add this example to user facing part of the documentation as well, so that users have access to the implementation code example.

sdadn · 2024-04-02T16:36:33Z

To make sure we are on the same page I wanted to double-check that subsections are going to be supported on table only in not supported in Grids. If that's correct, that should be also mentioned in documentation. In case I am wrong and subsections are supported in grids, there should be grid examples and tests added.

I believe subsections are only for flowsheet datagrids (@mjpalazzo is that correct?). Consuming these changes in flowsheet will be done in a separate PR.

mjpalazzo · 2024-04-02T17:42:08Z

To make sure we are on the same page I wanted to double-check that subsections are going to be supported on table only in not supported in Grids. If that's correct, that should be also mentioned in documentation. In case I am wrong and subsections are supported in grids, there should be grid examples and tests added.

I believe subsections are only for flowsheet datagrids (@mjpalazzo is that correct?). Consuming these changes in flowsheet will be done in a separate PR.

That is correct. Flowsheet only

cm9361 · 2024-04-02T17:47:07Z

There doesn't appear to be a new or updated example to show how to implement subsections.

cm9361 · 2024-04-02T17:48:49Z

packages/terra-table/src/clinical-lowlight-theme/SubSection.module.scss

+  .orion-fusion-theme {
+    --terra-table-subsection-font-size: 0.808571428571rem;
+    --terra-table-subsection-font-weight: 700;
+    --terra-table-subsection-font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;


Wouldn't we be using the base font family? I wouldn't expect us to have to define this in our sass file.

Removed here: 6ee7c59

cm9361 · 2024-04-02T17:50:11Z

packages/terra-table/src/subcomponents/SubSection.module.scss

+    padding-top: 0.3571428571rem;
+    padding-bottom: 0.3571428571rem;


I would expect the padding to be themeable as well.

Added here: 29bba90

cm9361 · 2024-04-02T17:51:34Z

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

+          <h3
+            className={cx('subsection')}
+          >
+            <span
+              className={cx('sticky')}
+            >
+              {text}
+            </span>
+          </h3>


Shouldn't we be using the SectionHeader Terra component here?

I believe @BenBoersma tried that but terra section header doesn't have the subsection styling guidelines provided by UX.

Disregard this, managed to override the styles of terra-section-header.

cm9361 · 2024-04-02T17:53:47Z

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

+          </h3>
+        </th>
+      </tr>
+    </>


Where is the content for the subsection?

cm9361 · 2024-04-02T17:54:53Z

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

+              {!isCollapsed && subsection.rows.map((row, rowIndex) => (
+                <Row
+                  rowIndex={subsection.subSectionRowIndex + (rowIndex + 1)}
+                  key={row.id}
+                  height={rowHeight}
+                  id={row.id}
+                  sectionId={!isHidden ? id : undefined}
+                  tableId={tableId}
+                  cells={row.cells}
+                  ariaLabel={row.ariaLabel}
+                  rowSelectionMode={rowSelectionMode}
+                  displayedColumns={displayedColumns}
+                  rowHeaderIndex={rowHeaderIndex}
+                  onCellSelect={onCellSelect}
+                  isSelected={row.isSelected}
+                  isTableStriped={isTableStriped}
+                  rowMinimumHeight={rowMinimumHeight}
+                  firstRowId={firstRowId}
+                  lastRowId={lastRowId}
+                />
+              ))}


The SubSection component should be rendering its own rows.

sdadn · 2024-04-03T11:32:20Z

There doesn't appear to be a new or updated example to show how to implement subsections.

@cm9361 This PR only adds subsection support for terra-table. Consuming that in flowsheet and adding documentation for consumers will be done in the next PR.

cm9361 · 2024-04-09T14:17:07Z

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

@@ -387,6 +393,7 @@ function Cell(props) {
  const columnHeaderId = `${tableId}-${columnId}-headerCell`;
  const rowHeaderId = !isRowHeader && rowHeaderIndex !== -1 ? `${tableId}-rowheader-${rowId} ` : '';
  const sectionHeaderId = sectionId ? `${tableId}-${sectionId} ` : '';
+  const subsectionHeaderId = subsectionId ? `${tableId}-${sectionId}-${subsectionId} ` : '';


Do we need the section id in the id for the subsection?

I added it so we'll know which section the subsection is associated with. Do we need that or is the section id redundant?

I guess having the section helps to ensure uniqueness.

cm9361 · 2024-04-09T14:20:40Z

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

It does not appear that the onCellSelect method has been updated to include the subsection id.

fixed here: a981a90

cm9361 · 2024-04-09T14:23:58Z

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

+    tableId,
+  };
+
+  if (subsections) {


What happens if the section has its own rows as well as subsections? I don't believe that use case is covered in this design.

From my understanding, a section would have either rows or subsections but not both. If both are passed in, then the rows will be ignored and not rendered. Does that meet the requirements? cc @mjpalazzo .

Subsections definitely can have rows. See screen shot.

Here is a link to a video that shows the full workflow:
https://oracle-my.sharepoint.com/:v:/r/personal/sriraksha_k_oracle_com/Documents/Orders%20for%20Signature%20Accessibility/Routing%20Section%20issue%20in%20OFS.mov?csf=1&web=1&e=pJAyt6&nav=eyJwbGF5YmFja09wdGlvbnMiOnt9LCJyZWZlcnJhbEluZm8iOnsicmVmZXJyYWxBcHAiOiJTdHJlYW1XZWJBcHAiLCJyZWZlcnJhbE1vZGUiOiJtaXMiLCJyZWZlcnJhbFZpZXciOiJwb3N0cm9sbC1jb3B5bGluayIsInJlZmVycmFsUGxheWJhY2tTZXNzaW9uSWQiOiI4MmMxMWE2ZC02YjQ3LTQ5MTgtOTRhZC1lNzViYWU5YzUxYTUifX0%3D

The subsections are used to show the routing of medication orders. There is typically a row under the subsection for each medication that qualifies for that route; e.g., electronic prescription, pharmacy call, etc.

Oh I meant if section level row and subsections are both simultaneously possible?

e.g.

section header

section row

section row

subsection

subsection row

subsection row 2

I will have to reach out the Clinical product manager for that question.

cm9361 · 2024-04-09T14:25:40Z

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

+                    text={subsection.text}
+                    isTitleSticky
+                    boundedWidth={boundingWidth}
+                    level={3}


Seems like the value of 3 should be a constant so that we can give a meaningful name to the value.

Added constant in a018196

cm9361 · 2024-04-09T14:28:34Z

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

+              collapsed: isCollapsed,
+              collapsible: isCollapsible,


Subsections are not collapsible. We should not need classes on the subsection. The subsection visibility should be covered by the parent section. We shouldn't even render subsections or their header rows if the section is collapsed.

Subsections don't get rendered, as confirmed by this test. But I can remove these classes if they are redundant.

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

Lines 193 to 197 in 9270557

it('validates a table with with collapsed sections', () => {

browser.url('/raw/tests/cerner-terra-framework-docs/table/table-with-collapsible-sections-and-sub-sections');

expect(browser.$('//*[@id="table-with-sub-sections"]/tbody[3]')).toHaveChildren(2);

Terra.validates.element('table-with-collasped-subsections', { selector: tableWithSubSectionsSelector });

});

Removed redundant css here: 0fc369b

cm9361 · 2024-04-09T14:39:41Z

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

+                >
+
+                  <SectionHeader
+                    className={cx('subsection')}


Shouldn't this class be something like subsection-header-row to match the section classes?

This appends the subsection class to the existing header-row class, so it would be header-row subsection, but I can rename it if it's preferred.

Renamed it here: e94e26b

cm9361 · 2024-04-09T14:40:03Z

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

+                </th>
+              </tr>
+            </tbody>
+            <tbody className={cx('subsection-rows', {


It seems like this would be the subsection class.

Looks like this is not needed, removed here: 0fc369b

cm9361 · 2024-04-09T14:49:41Z

Doesn't the isSection method in the DataGrid component need to be updated to account for the new subsections? If that is done, does that fix all your keyboard navigation issues?

cm9361 · 2024-04-09T15:13:16Z

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

+              <tr
+                aria-rowindex={subsection.subSectionRowIndex}
+                className={cx('header-row', theme.className)}
+                data-section-id={id}


Should this be data-subsection-id or something similar? This would not technically be a section, right?

Fixed here: d1c988e

sdadn · 2024-04-09T16:32:55Z

Some CSS observations

Lowlight theme styling looks off to me:

NIT (if that is expect behavior - please disregard it), subsection header text alignment is not exactly aligned with section header (or anything else) on the left. It is placed much more to the right than the rest of the content.

@adoroshk

I have corrected the lowlight styling, thanks for pointing that out!
The offset is by design from UX.

sdadn · 2024-04-09T16:49:35Z

Doesn't the isSection method in the DataGrid component need to be updated to account for the new subsections? If that is done, does that fix all your keyboard navigation issues?

@cm9361 I haven't been able to work on the DataGrid component yet. So far, I've mainly been focusing on adding support for subsections to the Table component for this Jira & PR.

BenBoersma added 4 commits March 27, 2024 16:01

initial changes for adding subsections to table

e14fa46

adding wdio tests and snapshot for default theme

a6276c7

added fusion-theme variables, added wdio tests, added jest tests, fix…

ce6cc6b

…ed an aria-rowindex error in table

Updating to fix linter errors

b3a7c15

BenBoersma added the 📦 terra-table label Mar 29, 2024

BenBoersma self-assigned this Mar 29, 2024

updated wdio tests

cc25304

sdadn marked this pull request as ready for review April 1, 2024 15:27

sdadn self-assigned this Apr 1, 2024

adoroshk reviewed Apr 2, 2024

View reviewed changes

sdadn added 2 commits April 2, 2024 11:47

Merge branch 'main' into adding-table-subsections

7972bd5

removed linter comments

86bd928

github-actions bot temporarily deployed to preview-pr-2112 April 2, 2024 17:14 Destroyed

cm9361 reviewed Apr 2, 2024

View reviewed changes

packages/terra-table/src/subcomponents/SubSection.jsx Outdated

</h3>

</th>

</tr>

</>

Copy link

Contributor

cm9361 Apr 2, 2024

Choose a reason for hiding this comment

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

Where is the content for the subsection?

cm9361 reviewed Apr 2, 2024

View reviewed changes

added variables for subsection padding

29bba90

github-actions bot temporarily deployed to preview-pr-2112 April 3, 2024 11:30 Destroyed

removed font family

6ee7c59

github-actions bot temporarily deployed to preview-pr-2112 April 3, 2024 11:48 Destroyed

sdadn added 2 commits April 3, 2024 06:59

updated subsection logic

586ed87

updated section logic

1124ed0

github-actions bot temporarily deployed to preview-pr-2112 April 3, 2024 18:51 Destroyed

linter fixes

67b7f7d

github-actions bot deployed to preview-pr-2112 April 9, 2024 13:29 View deployment

linter fixes

9270557

github-actions bot temporarily deployed to preview-pr-2112 April 9, 2024 13:33 Destroyed

cm9361 reviewed Apr 9, 2024

View reviewed changes

added constant

a018196

cm9361 reviewed Apr 9, 2024

View reviewed changes

sdadn added 3 commits April 9, 2024 10:23

updated onCellSelect callback

a981a90

updated subsection classame

e94e26b

fix subsection sticky title

d54279f

github-actions bot temporarily deployed to preview-pr-2112 April 9, 2024 15:36 Destroyed

sdadn added 2 commits April 9, 2024 11:14

updated data attribute

d1c988e

removed redundant css

0fc369b

github-actions bot temporarily deployed to preview-pr-2112 April 9, 2024 16:27 Destroyed

Revert padding

a357020

github-actions bot temporarily deployed to preview-pr-2112 April 9, 2024 18:22 Destroyed

cm9361 approved these changes Apr 9, 2024

View reviewed changes

fixed wdio snapshots

8061321

github-actions bot temporarily deployed to preview-pr-2112 April 9, 2024 21:30 Destroyed

sdadn merged commit c44438d into main Apr 9, 2024
20 of 21 checks passed

sdadn deleted the adding-table-subsections branch April 9, 2024 21:56

		padding-top: 0.3571428571rem;
		padding-bottom: 0.3571428571rem;

	it('validates a table with with collapsed sections', () => {
	browser.url('/raw/tests/cerner-terra-framework-docs/table/table-with-collapsible-sections-and-sub-sections');
	expect(browser.$('//*[@id="table-with-sub-sections"]/tbody[3]')).toHaveChildren(2);
	Terra.validates.element('table-with-collasped-subsections', { selector: tableWithSubSectionsSelector });
	});

[terra-table] Adding subsections to sections #2112

[terra-table] Adding subsections to sections #2112

Conversation

BenBoersma commented Mar 29, 2024

Summary

Testing

Reviews

Additional Details

adoroshk left a comment • edited Loading

Choose a reason for hiding this comment

adoroshk left a 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

sdadn commented Apr 2, 2024

mjpalazzo commented Apr 2, 2024

cm9361 commented Apr 2, 2024

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

sdadn commented Apr 3, 2024

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

cm9361 Apr 9, 2024 • 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

sdadn Apr 9, 2024 • 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

cm9361 commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn commented Apr 9, 2024

sdadn commented Apr 9, 2024

adoroshk left a comment •

edited

Loading

cm9361 Apr 9, 2024 •

edited

Loading

sdadn Apr 9, 2024 •

edited

Loading