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

[flowsheet-data-grid] Hide columns headers #1882

Merged
merged 36 commits into from
Nov 16, 2023
Merged

Conversation

sdadn
Copy link
Contributor

@sdadn sdadn commented Nov 8, 2023

Summary

This PR hides the column headers in the flowsheet-data-grid to meet functional requirements.

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

This PR resolves:

UXPLATFORM-9771


Thank you for contributing to Terra.
@cerner/terra

@sdadn sdadn marked this pull request as ready for review November 8, 2023 21:20
@@ -295,6 +295,7 @@ function FlowsheetDataGrid(props) {
onCellSelect={handleCellSelection}
onClearSelection={handleClearSelectedCells}
onCellRangeSelect={handleCellRangeSelection}
hasColumnHeaders={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

The Flowsheet should have column headers by default. The prop should be able to be set by consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I misunderstood the requirements then. I'll change it.

@chrismichalewicz
Copy link

@sdadn Based on Charles's comment, and your response, looks like you are addressing this. But from a documentation/examples perspective I think we should have a specific example without column headers and then keep the others to have headers.

Comment on lines 13 to 14
* Changed
* Updated FlowsheetDataGrid to no longer have column headers.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will now go under the Added section. Also, you are allowing the column headers to be hidden. However, it is still possible. Maybe this update was missed based on your other updates in the PR.

import React, { useState, useCallback } from 'react';
import { FlowsheetDataGrid } from 'terra-data-grid';

const DefaultFlowsheetDataGrid = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be ColumnHeadersHidden instead of DefaultFlowsheetDataGrid.

@sdadn
Copy link
Contributor Author

sdadn commented Nov 9, 2023

@eawww , @chrismichalewicz I added the no header example, so it should now be ready for review.

@chrismichalewicz
Copy link

chrismichalewicz commented Nov 9, 2023

@eawww , @chrismichalewicz I added the no header example, so it should now be ready for review.

@sdadn Did you update the right example?
The "Columns Header Hidden" example is showing headers, it looks like the "No Results Cell" might have been updated by mistake as it shows with no headers.

Also can "Columns Header Hidden" be updated to "Column Headers Hidden"?

@@ -376,6 +385,7 @@ const DataGrid = injectIntl((props) => {
const key = event.keyCode;
switch (key) {
case KeyCode.KEY_UP:
if (!hasVisibleColumnHeaders && nextRow - 1 === 0) break;
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 think you could move this logic to this block of code
if (nextCol < 0 || nextRow < 0) {
event.preventDefault(); // prevent the page from moving with the arrow keys.
return;
}

Your current implementation does not account for preventing the default behavior so that the page does not scroll. The basis of that logic is already lower in the file and checking for the top boundary. You would just have to modify the condition to also handle the hidden column header use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here: 766fbf5

Comment on lines +3 to +16
# Hiding Column Headers

### Description
The [Flowsheet Data Grid](/components/cerner-terra-framework-docs/data-grid/flowsheet-data-grid/about) can hide column headers with the `hasVisibleColumnHeaders` prop.

### Usage
The `hasVisibleColumns` is a boolean. By default, it is set to `true`.
When it is set to `false`, the column headers are hidden, but still exist in the DOM and can be read by screenreaders when providing context for the table.
This is useful when needing to use custom alternative to column headers, such as Stella Timeline.

**Note:**
- Column header labels should still be provided in the dataset to ensure that sceenreaders will still read out the appropriate context for the table.
- When using custom column headers, then logic must be added to ensure that the column widths match are in sync if resizing columns are enabled.
The [columns.width](/components/cerner-terra-framework-docs/data-grid/flowsheet-data-grid/about#column) property for columns can be used to set the column widths programatically.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this should be on the About page instead of an example page. You would not want consumers to miss this information because they did not select one of the examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I'm just adding the necessary information. It can be reorganized in the implementation guide story which will be updating the documentation as a whole and making sure all pieces fit together.

@sdadn sdadn added ⭐ UX Reviewed UX Reviewed and approved. and removed UX Review Required labels Nov 15, 2023
@github-actions github-actions bot temporarily deployed to preview-pr-1882 November 15, 2023 23:08 Destroyed
@sdadn sdadn merged commit 989a87d into main Nov 16, 2023
21 checks passed
@sdadn sdadn deleted the hide-flowsheet-column-header branch November 16, 2023 16:09
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.

5 participants