feat: Table Column Groups Table#4482
Conversation
40c20ee to
84451af
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4482 +/- ##
==========================================
+ Coverage 97.41% 97.44% +0.02%
==========================================
Files 933 940 +7
Lines 29595 30014 +419
Branches 10757 10946 +189
==========================================
+ Hits 28831 29248 +417
- Misses 716 759 +43
+ Partials 48 7 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
84451af to
e9f4fd0
Compare
d6a98cb to
5cec8c8
Compare
8da3b07 to
227f21b
Compare
| totalLeafColumns: number | ||
| ): GroupSplit | null { | ||
| if (!col.isGroup) { | ||
| return null; |
There was a problem hiding this comment.
Can we make this more generalized and always return the structure here instead of null (in that case the stickyColumns will equal 0, but then the consumer code will not have to do a null check - so one code branch less).
| return useMemo(() => { | ||
| const visibleIds = visibleColumns | ||
| ? Array.from(visibleColumns) | ||
| : columnDefinitions.map((col, idx) => col.id || `column-${idx}`); |
There was a problem hiding this comment.
We already have the getColumnKey() to be used instead of a custom col.id || column-${idx}
| [...columnDefinitions], | ||
| visibleIds, | ||
| [...(groupDefinitions ?? [])], | ||
| columnDisplay ? [...columnDisplay] : undefined |
There was a problem hiding this comment.
We do we copy columnDisplay here? Can we change calculateHierarchyTree to accept it as readonly instead? Same for column definitions above.
| visibleColumns?: Set<string>, | ||
| columnDisplay?: ReadonlyArray<TableProps.ColumnDisplayProperties> | ||
| ) { | ||
| return useMemo(() => { |
There was a problem hiding this comment.
Is useMemo really needed here - do we expect calculateHierarchyTree to be heavy in computations?
| import { TableProps } from '../interfaces'; | ||
| import { getVisibleColumnDefinitions } from '../utils'; | ||
|
|
||
| export interface ColumnInRow<T> { |
There was a problem hiding this comment.
nit: should we rename it to HeaderRowColumn? Then HeaderRow and HeaderRowColumn are better symmetrical
| columnDefinition?: TableProps.ColumnDefinition<T>; | ||
| groupDefinition?: TableProps.GroupDefinition; | ||
| parentGroupIds: string[]; | ||
| rowIndex: number; |
There was a problem hiding this comment.
should rowIndex belong to the HeaderRow?
| if (item.type === 'group') { | ||
| const groupNode = nodeMap.get(item.id); | ||
| if (!groupNode) { | ||
| if (isDevelopment) { |
There was a problem hiding this comment.
the warnOnce already checks for isDevelopment - there is no need to add a separate check
| continue; | ||
| } | ||
| buildTreeFromColumnDisplay(item.children, nodeMap, groupNode); | ||
| if (groupNode.children.length > 0) { |
There was a problem hiding this comment.
I think this line deserves a code comment to explain why we check for children to be non-empty. Btw, can children array include group nodes - or only column nodes? If it can include groups - then this check is not sufficient.
There was a problem hiding this comment.
Also, can groupNode.children include column nodes that are not visible with column display?
| root: TableHeaderNode<T> | ||
| ): void { | ||
| for (const col of visibleColumns) { | ||
| /* istanbul ignore next */ |
There was a problem hiding this comment.
Why do we ignore it? We once had a high-sev issue, only reproducible for cases where col.id was not set.
| const nodeMap = new Map<string, TableHeaderNode<T>>(); | ||
|
|
||
| for (const col of visibleColumns) { | ||
| if (col.id) { |
There was a problem hiding this comment.
I would add a code comment explaining why ignoring columns w/o IDs is ok
| nodeMap.set(group.id, new TableHeaderNode<T>(group.id, { groupDefinition: group })); | ||
| } | ||
|
|
||
| // Build tree |
There was a problem hiding this comment.
nit: The comments like "Build tree" or "Compute layout" are redundant - the corresponding code (new TableHeaderNode(...), computeSubTreeHeights(root)) is clear enough.
| if (columnDisplay && columnDisplay.length > 0) { | ||
| buildTreeFromColumnDisplay(columnDisplay, nodeMap, root); | ||
| } else { | ||
| connectFlatColumns(visibleColumns, nodeMap, root); |
There was a problem hiding this comment.
Why do we call this connectFlatColumns? Should we call it buildTreeFromVisibleColumns - for the symmetry with the other method? This one involves much simpler transformations, but these are transformations nevertheless.
| } | ||
|
|
||
| const rows: HeaderRow<T>[] = Array.from(rowsMap.keys()) | ||
| .sort((a, b) => a - b) |
There was a problem hiding this comment.
why do we perform sorting twice on the same array?
|
|
||
| import styles from './styles.css.js'; | ||
|
|
||
| export interface TableGroupHeaderCellProps { |
There was a problem hiding this comment.
Can we share common props with TableHeaderCellProps?
| /** When true, this cell is the rightmost child within its parent group. */ | ||
| isLastChildOfGroup?: boolean; | ||
| /** Determine if the cell is the right most cell of the header */ | ||
| isRightmost?: boolean; |
There was a problem hiding this comment.
We should not call it isRightmost - as it will be left-most when using RTL direction. I suggest calling this isLastColumn - we already use this name inside table's code.
There was a problem hiding this comment.
If the word "column" does not quite apply here semantically - maybe we can just use "isLast"
| }); | ||
|
|
||
| // Extract only the shadow classes from the boundary subscription | ||
| /* istanbul ignore next: requires real sticky column state */ |
There was a problem hiding this comment.
What is real sticky column state? Does it require some browser behaviours?
| columnGroupId?: string; | ||
| isRightmost?: boolean; | ||
| /** Additional className to merge (e.g. boundary shadow classes from a secondary sticky subscription). */ | ||
| extraClassName?: string; |
There was a problem hiding this comment.
We should call it just className - that is commonly used when we need to attach some classes from the outside.
| /** Additional className to merge (e.g. boundary shadow classes from a secondary sticky subscription). */ | ||
| extraClassName?: string; | ||
| /** Additional ref for boundary sticky subscription (imperatively updates shadow classes). */ | ||
| extraRef?: React.RefCallback<HTMLElement>; |
There was a problem hiding this comment.
Why is this called extraRef? Is it not just the ref we attach to the th el?
| stickyColumns: StickyColumnsModel; | ||
| columnId: PropertyKey; | ||
| getClassName: (styles: null | StickyColumnsCellState) => Record<string, boolean>; | ||
| classOnly?: boolean; |
| let targetCell = | ||
| allVisibleCells.length > 0 | ||
| ? findClosestCellByAriaColIndex(allVisibleCells, targetAriaColIndex, delta.x) | ||
| : /* istanbul ignore next */ findTableRowCellByAriaColIndex(targetRow, targetAriaColIndex, delta.x); |
There was a problem hiding this comment.
Why is this ignored? The keyboard nav should be testable and tested with unit tests
| targetCell = | ||
| allVisibleCells.length > 0 | ||
| ? findClosestCellByAriaColIndex(allVisibleCells, targetAriaColIndex, delta.x) | ||
| : /* istanbul ignore next */ findTableRowCellByAriaColIndex(skipRow, targetAriaColIndex, delta.x); |
| // Jump to the first row after this cell's span (↓) or one row before the cell's start (↑). | ||
| const skipToRowIndex = delta.y > 0 ? cellRowIndex + cellRowSpan : cellRowIndex - 1; | ||
| const skipRow = findTableRowByAriaRowIndex(this.table, skipToRowIndex, delta.y); | ||
| /* istanbul ignore next */ if (!skipRow) { |
| targetCell = findClosestCellByAriaColIndex(allVisibleCells, skipToColIndex, delta.x); | ||
| if (!targetCell || targetCell === cellElement) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The changes in this file adds lots of complexity. What is the behaviour w/o this changes?
There was a problem hiding this comment.
I think there is an opportunity to significantly simplify the code here. Theoretically, the findCell by next col/row index utils should locate the correct element to focus, and might require changes to be aware of col/row spans. It is not clear why changes in the grid nav logic are necessary.
| * are only in one <tr> in the DOM but visually occupy multiple rows. | ||
| */ | ||
| export function getAllCellsInRow(table: null | HTMLTableElement, targetAriaRowIndex: number): HTMLTableCellElement[] { | ||
| /* istanbul ignore next */ if (!table) { |
There was a problem hiding this comment.
We should test this, not ignore
There was a problem hiding this comment.
I guess the problem is that we expect table to always be there - so it is kind of hard to make it be null for this util call. In that case we might check for the table before calling the util instead.
|
|
||
| const GRID_NAVIGATION_PAGE_SIZE = 10; | ||
| const SELECTION_COLUMN_WIDTH = 54; | ||
| const SELECTION_COLUMN_WIDTH = 40; |
| }); | ||
|
|
||
| // Build visible column IDs set for grouping | ||
| const visibleColumnIds = new Set(visibleColumnDefinitions.map((col, idx) => col.id || `column-${idx}`)); |
There was a problem hiding this comment.
We should use getColumnKey() here
| {...getTableRoleProps({ tableRole })} | ||
| > | ||
| {hasGroupedColumns && columnDefinitions && ( | ||
| <colgroup> |
There was a problem hiding this comment.
should we also use <TableColGroup /> here?
| const { getColumnStyles, columnWidths, updateColumn, setCell } = useColumnWidths(); | ||
| const { getColumnStyles, columnWidths, updateColumn, updateGroup, setCell } = useColumnWidths(); | ||
|
|
||
| /* istanbul ignore next: resize requires real DOM measurements */ |
There was a problem hiding this comment.
I would try to find a way and cover it with unit tests to be safe
| const handleSplitGroupResize = (leafIds: string[], newWidth: number) => { | ||
| const lastLeaf = leafIds[leafIds.length - 1]; | ||
| if (lastLeaf) { | ||
| const currentHalfWidth = leafIds.reduce((sum, id) => sum + (columnWidths.get(id) || 120), 0); |
Description
Add Table Column Grouping Feature support for Table Component.
Related links, issue AWSUI-9594, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.