[terra-data-grid] DataGrid consumes Table #1851
Conversation
…ing the necessary coordinates. Reduce frequency of pinned columns warning.
kenk2
added
📦 terra-worklist-data-grid
📦 terra-data-grid
and removed
📦 terra-worklist-data-grid
labels
| :focus { | ||
| outline: var(--terra-data-grid-cell-focus-outline, 3px dashed #000); | ||
| outline-offset: var(--terra-data-grid-cell-focus-outline-offset, -2px); |
There was a problem hiding this comment.
This focus selector should not have been removed.
There was a problem hiding this comment.
It appears that this is still removed.
There was a problem hiding this comment.
So it turns out that this is in table, so we don't actually need this here after all.
| /** | ||
| * Boolean specifying whether or not the component should have zebra striping for rows. | ||
| */ | ||
| isStriped: PropTypes.bool, |
There was a problem hiding this comment.
This should not be a prop for the Flowsheet. The Flowsheet should always be striped.
There was a problem hiding this comment.
Moved the striped logic out of the Flowsheet and WDG. Now only specified in DataGrid.
| @@ -94,6 +101,7 @@ function FlowsheetDataGrid(props) { | |||
| id={id} | |||
| ariaLabel={ariaLabel} | |||
| ariaLabelledBy={ariaLabelledBy} | |||
| isStriped={isStriped} | |||
There was a problem hiding this comment.
We should always be passing true. The logic should be done in the DataGrid component. We should not need this logic in either the WorklistDataGrid or Flowsheet components.
| /** | ||
| * Boolean specifying whether or not the component should have zebra striping for rows. | ||
| */ | ||
| isStriped: PropTypes.bool, |
There was a problem hiding this comment.
This should not be a prop for the WorklistDataGrid. The WorklistDataGrid should always be striped.
| @@ -418,11 +425,26 @@ function WorklistDataGrid(props) { | |||
| id={id} | |||
| ariaLabel={ariaLabel} | |||
| ariaLabelledBy={ariaLabelledBy} | |||
| rows={rows} | |||
| isStriped={isStriped} | |||
There was a problem hiding this comment.
This value should always be true. The logic should be in the DataGrid component.
| isSelectable: columns.isSelectable !== false, | ||
| isResizable: columns.isResizable !== false, | ||
| }))} | ||
| overflowColumns={overflowColumns.map((columns) => ({ |
There was a problem hiding this comment.
Wouldn't the iterator variable name be "column"? Isn't this an individual column that is being passed to the function?
There was a problem hiding this comment.
Created some mapper functions to apply this to Datagrid and WDG.
| rows={rows.map((row) => ({ | ||
| ...row, | ||
| cells: row.cells.map(cell => ({ | ||
| ...cell, | ||
| isSelectable: cell.isSelectable !== false, | ||
| })), | ||
| }))} |
There was a problem hiding this comment.
It might be better for maintenance if we create a function for this.
| ...columns, | ||
| isSelectable: columns.isSelectable !== false, | ||
| isResizable: columns.isResizable !== false, |
There was a problem hiding this comment.
Looks like we are duplicating the same logic for columns. Maybe it should be a function.
packages/terra-table/src/Table.jsx
Outdated
| useEffect(() => { | ||
| if (pinnedColumns.length === 0) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn(ERRORS.PINNED_COLUMNS_UNDEFINED); | ||
| } | ||
| }, [pinnedColumns.length]); |
There was a problem hiding this comment.
We would not want the update on the length of the columns. We just need it to update when pinnedColumns does.
There was a problem hiding this comment.
Removed this and the sending of the cell coordinates.
kenk2
changed the title
| @@ -5,6 +5,7 @@ | |||
| * Fixed | |||
| * Fixed the column header background color for the Orion Fusion theme. | |||
| * Fixed pinned column divider issue during keyboard resize. | |||
| * Fixed left column header border for components. | |||
There was a problem hiding this comment.
Did you rebase with main? This code should already be present and not show up as a change.
There was a problem hiding this comment.
This change was not made in datagrid (which still has this bug), so I had to fix it in this PR. We're going to consume from table from now on so the fix was needed here.
There was a problem hiding this comment.
There is no need to update the data grid logic. We won't be using it.
| @@ -9,19 +9,7 @@ | |||
| width: 100%; | |||
There was a problem hiding this comment.
I would not expect the data-grid-container element to have an overflow. The Terra Table already had an element with an overflow.
| const TABLE_ROW_SELECTION_COLUMN = { | ||
| id: 'table-rowSelectionColumn', | ||
| width: 40, | ||
| isSelectable: true, | ||
| isResizable: false, | ||
| }; |
There was a problem hiding this comment.
We should not need this logic. It is encapsulated in the Terra Table.
| const WorklistDataGridUtils = { | ||
| ROW_SELECTION_COLUMN, | ||
| TABLE_ROW_SELECTION_COLUMN, | ||
| }; |
There was a problem hiding this comment.
Do we still need this logic with the updated Terra Table?
There was a problem hiding this comment.
We do because we're using different ID's for the row selection column in each component. Table uses table-rowSelectionColumn while WDG uses worklistDataGrid-rowSelectionColumn. I also want to put this here because we reference this in multiple places so we don't have magic strings. I could however as an alternative, import this from terra table.
There was a problem hiding this comment.
You don't need this entire structure just for an id.
| { "id": "Column-0", "displayName": "Vitals", "isSelectable": true, "sortIndicator": "ascending", "isResizable": true }, | ||
| { "id": "Column-1", "displayName": "March 16", "isSelectable": true, "isResizable": true }, | ||
| { "id": "Column-2", "displayName": "March 17", "isSelectable": true }, |
There was a problem hiding this comment.
Why did this need to change for the Terra Table?
There was a problem hiding this comment.
This was just to provide a way for people to test column resizing and selectable columns in table.
| @@ -42,6 +42,7 @@ const SortableTable = () => { | |||
| id="sortable-table" | |||
| overflowColumns={tableColumns} | |||
| rows={tableRows} | |||
| columnResizeIncrement={5} | |||
There was a problem hiding this comment.
This should not have required a change since we have a default value defined.
packages/terra-table/CHANGELOG.md
Outdated
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| * Fixed | |||
| * Fixed the column header background color for the Orion Fusion theme. | |||
| * Fixed an issue with row selection integrating with table zebra stripes. | |||
There was a problem hiding this comment.
This should not show up as your change.
sdadn
added
⭐ UX Reviewed
…tion column headers.
| <GridContext.Provider value={{ | ||
| role: GridConstants.GRID, | ||
| setCellAriaLiveMessage, | ||
| tableRef: grid, | ||
| tableContainerRef, | ||
| }} |
There was a problem hiding this comment.
You did not memoize your provider value. This will cause many unnecessary rerenders.
| @@ -64,21 +66,22 @@ afterAll(() => { | |||
|
|
|||
| describe('DataGrid', () => { | |||
| it('verifies that the grid created is consistent with the rows and overflowColumns props', () => { | |||
| const wrapper = shallowWithIntl( | |||
| const wrapper = mountWithIntl( | |||
There was a problem hiding this comment.
Was there a reason these needed to be changed to mount?
There was a problem hiding this comment.
I attempted to make this work with the existing shallow render but was not able to find the components with shallow alone as Table is now embedded in a Host Component, thus making us unable to find the component without fully mounting it.
Summary
What was changed:
DataGrid consumes table.
Why it was changed:
To eliminate redundant code and follow the DataGrid roadmap
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-9543
Thank you for contributing to Terra.
@cerner/terra