From e9b54b828e3a02f593c5cd01e73c49a40875cae8 Mon Sep 17 00:00:00 2001 From: Adhitya Mamallan Date: Fri, 22 Nov 2024 14:27:20 +0530 Subject: [PATCH 1/6] Make onSort required only if the table config has at least 1 sortable column --- src/components/table/table.tsx | 9 ++++++--- src/components/table/table.types.ts | 14 +++++++++++--- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/components/table/table.tsx b/src/components/table/table.tsx index fce422f13..c25b1e5a2 100644 --- a/src/components/table/table.tsx +++ b/src/components/table/table.tsx @@ -10,15 +10,18 @@ import { import TableSortableHeadCell from './table-sortable-head-cell/table-sortable-head-cell'; import { styled } from './table.styles'; -import type { Props } from './table.types'; +import type { Props, TableColumn } from './table.types'; -export default function Table({ +export default function Table< + T extends object, + C extends Array>, +>({ data, columns, shouldShowResults, endMessage, ...sortParams -}: Props) { +}: Props) { return ( diff --git a/src/components/table/table.types.ts b/src/components/table/table.types.ts index 70dbdef78..a9cac0fc7 100644 --- a/src/components/table/table.types.ts +++ b/src/components/table/table.types.ts @@ -8,13 +8,21 @@ export type TableColumn = { sortable?: boolean; }; -export type Props = { +type IsTableConfigSortable>> = true extends { + [K in keyof C]: C[K] extends { sortable: true } ? true : false; +}[number] + ? true + : false; + +export type Props>> = { data: Array; - columns: Array>; + columns: C; shouldShowResults: boolean; endMessage: React.ReactNode; // Sort params - onSort: (column: string) => void; + onSort: IsTableConfigSortable extends true + ? (column: string) => void + : never; sortColumn?: string; sortOrder?: SortOrder; }; From 3c90a994ddee809fd543fb7975e0c4adeff85d56 Mon Sep 17 00:00:00 2001 From: Adhitya Mamallan Date: Fri, 22 Nov 2024 15:52:45 +0530 Subject: [PATCH 2/6] Make changes to tests --- src/components/table/__tests__/table.test.tsx | 54 +++++++++++++++---- .../table-sortable-head-cell.tsx | 2 +- src/components/table/table.tsx | 8 ++- src/components/table/table.types.ts | 17 +++--- .../config/domain-workflows-table.config.ts | 4 +- .../config/domains-table-columns.config.ts | 6 +-- .../domains-table/domains-table.types.ts | 2 +- .../config/task-list-workers-table.config.ts | 2 +- 8 files changed, 64 insertions(+), 31 deletions(-) diff --git a/src/components/table/__tests__/table.test.tsx b/src/components/table/__tests__/table.test.tsx index c3574e63e..1131b58f1 100644 --- a/src/components/table/__tests__/table.test.tsx +++ b/src/components/table/__tests__/table.test.tsx @@ -3,6 +3,7 @@ import React from 'react'; import { render, screen, fireEvent, act } from '@/test-utils/rtl'; import Table from '../table'; +import { type TableConfig } from '../table.types'; type TestDataT = { value: string; @@ -16,18 +17,49 @@ const SAMPLE_ROWS: Array = Array.from( (_, rowIndex) => ({ value: `test_${rowIndex}` }) ); -const SAMPLE_COLUMNS = Array.from( - { length: SAMPLE_DATA_NUM_COLUMNS }, - (_, colIndex) => ({ - name: `Column Name ${colIndex}`, - id: `column_id_${colIndex}`, +function getMockRenderCell(index: number) { + return ({ value }: TestDataT) => { + return `data_${value}_${index}`; + }; +} + +const SAMPLE_COLUMNS = [ + { + name: 'Column name 0', + id: 'column_id_0', sortable: true, - renderCell: ({ value }: TestDataT) => { - return `data_${value}_${colIndex}`; - }, - width: `${100 / SAMPLE_DATA_NUM_COLUMNS}%`, - }) -); + renderCell: getMockRenderCell(0), + width: '20%', + }, + { + name: 'Column name 1', + id: 'column_id_1', + sortable: true, + renderCell: getMockRenderCell(1), + width: '20%', + }, + { + name: 'Column name 2', + id: 'column_id_2', + sortable: true, + renderCell: getMockRenderCell(2), + width: '20%', + }, + { + name: 'Column name 3', + id: 'column_id_3', + sortable: true, + renderCell: getMockRenderCell(3), + width: '20%', + }, + { + name: 'Column name 4', + id: 'column_id_4', + sortable: true, + renderCell: getMockRenderCell(4), + width: '20%', + }, +] as const satisfies TableConfig; describe('Table', () => { it('should render without error', async () => { diff --git a/src/components/table/table-sortable-head-cell/table-sortable-head-cell.tsx b/src/components/table/table-sortable-head-cell/table-sortable-head-cell.tsx index 5a51af7a0..a64e04952 100644 --- a/src/components/table/table-sortable-head-cell/table-sortable-head-cell.tsx +++ b/src/components/table/table-sortable-head-cell/table-sortable-head-cell.tsx @@ -10,7 +10,7 @@ export default function TableSortableHeadCell({ name, columnID, width, - onSort, + onSort = () => {}, sortColumn, sortOrder, }: Props) { diff --git a/src/components/table/table.tsx b/src/components/table/table.tsx index c25b1e5a2..01f2b3cc1 100644 --- a/src/components/table/table.tsx +++ b/src/components/table/table.tsx @@ -10,12 +10,9 @@ import { import TableSortableHeadCell from './table-sortable-head-cell/table-sortable-head-cell'; import { styled } from './table.styles'; -import type { Props, TableColumn } from './table.types'; +import type { Props, TableConfig } from './table.types'; -export default function Table< - T extends object, - C extends Array>, ->({ +export default function Table>({ data, columns, shouldShowResults, @@ -34,6 +31,7 @@ export default function Table< name={column.name} columnID={column.id} width={column.width} + onSort={() => {}} {...sortParams} /> ) : ( diff --git a/src/components/table/table.types.ts b/src/components/table/table.types.ts index a9cac0fc7..2cf3e13a7 100644 --- a/src/components/table/table.types.ts +++ b/src/components/table/table.types.ts @@ -8,21 +8,24 @@ export type TableColumn = { sortable?: boolean; }; -type IsTableConfigSortable>> = true extends { +export type TableConfig = Array>; + +type AreAnyColumnsSortable> = true extends { [K in keyof C]: C[K] extends { sortable: true } ? true : false; }[number] ? true : false; -export type Props>> = { +type OnSortFunctionOptional> = + AreAnyColumnsSortable extends true + ? { onSort: (column: string) => void } + : { onSort?: (column: string) => void }; + +export type Props> = { data: Array; columns: C; shouldShowResults: boolean; endMessage: React.ReactNode; - // Sort params - onSort: IsTableConfigSortable extends true - ? (column: string) => void - : never; sortColumn?: string; sortOrder?: SortOrder; -}; +} & OnSortFunctionOptional; diff --git a/src/views/domain-workflows/config/domain-workflows-table.config.ts b/src/views/domain-workflows/config/domain-workflows-table.config.ts index 5392d67b5..09dbd8a69 100644 --- a/src/views/domain-workflows/config/domain-workflows-table.config.ts +++ b/src/views/domain-workflows/config/domain-workflows-table.config.ts @@ -2,7 +2,7 @@ import { createElement } from 'react'; import FormattedDate from '@/components/formatted-date/formatted-date'; import Link from '@/components/link/link'; -import { type TableColumn } from '@/components/table/table.types'; +import { type TableConfig } from '@/components/table/table.types'; import { type DomainWorkflow } from '@/views/domain-page/domain-page.types'; import WorkflowStatusTag from '@/views/shared/workflow-status-tag/workflow-status-tag'; @@ -55,6 +55,6 @@ const domainWorkflowsTableConfig = [ width: '12.5%', sortable: true, }, -] as const satisfies Array>; +] as const satisfies TableConfig; export default domainWorkflowsTableConfig; diff --git a/src/views/domains-page/config/domains-table-columns.config.ts b/src/views/domains-page/config/domains-table-columns.config.ts index d913d9dcd..24f7d35ff 100644 --- a/src/views/domains-page/config/domains-table-columns.config.ts +++ b/src/views/domains-page/config/domains-table-columns.config.ts @@ -1,10 +1,10 @@ -import { type TableColumn } from '@/components/table/table.types'; +import { type TableConfig } from '@/components/table/table.types'; import { type DomainData } from '../domains-page.types'; import DomainsTableClusterCell from '../domains-table-cluster-cell/domains-table-cluster-cell'; import DomainsTableDomainNameCell from '../domains-table-domain-name-cell/domains-table-domain-name-cell'; -const domainsTableColumnsConfig: Array> = [ +const domainsTableColumnsConfig = [ { name: 'Domain Name', id: 'name', @@ -18,6 +18,6 @@ const domainsTableColumnsConfig: Array> = [ renderCell: DomainsTableClusterCell, width: '20%', }, -]; +] as const satisfies TableConfig; export default domainsTableColumnsConfig; diff --git a/src/views/domains-page/domains-table/domains-table.types.ts b/src/views/domains-page/domains-table/domains-table.types.ts index d3878697c..c599ad712 100644 --- a/src/views/domains-page/domains-table/domains-table.types.ts +++ b/src/views/domains-page/domains-table/domains-table.types.ts @@ -2,7 +2,7 @@ import type { TableColumn } from '@/components/table/table.types'; import type { DomainData } from '../domains-page.types'; -export type DomainsTableColumns = Array>; +export type DomainsTableColumns = TableConfig; export type Props = { domains: Array; diff --git a/src/views/task-list-page/config/task-list-workers-table.config.ts b/src/views/task-list-page/config/task-list-workers-table.config.ts index 229b78a96..8d5c7d970 100644 --- a/src/views/task-list-page/config/task-list-workers-table.config.ts +++ b/src/views/task-list-page/config/task-list-workers-table.config.ts @@ -40,6 +40,6 @@ const taskListWorkersTableConfig = [ }), width: '10%', }, -] as const satisfies Array>; +] as const satisfies TableConfig; export default taskListWorkersTableConfig; From 9a7cdf1852cc34c81b105bdbaa2a65ba0b627f8e Mon Sep 17 00:00:00 2001 From: Adhitya Mamallan Date: Fri, 22 Nov 2024 16:26:39 +0530 Subject: [PATCH 3/6] Fix type errors --- src/views/domains-page/domains-table/domains-table.types.ts | 2 +- .../task-list-page/config/task-list-workers-table.config.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/views/domains-page/domains-table/domains-table.types.ts b/src/views/domains-page/domains-table/domains-table.types.ts index c599ad712..7f258a7be 100644 --- a/src/views/domains-page/domains-table/domains-table.types.ts +++ b/src/views/domains-page/domains-table/domains-table.types.ts @@ -1,4 +1,4 @@ -import type { TableColumn } from '@/components/table/table.types'; +import type { TableConfig } from '@/components/table/table.types'; import type { DomainData } from '../domains-page.types'; diff --git a/src/views/task-list-page/config/task-list-workers-table.config.ts b/src/views/task-list-page/config/task-list-workers-table.config.ts index 8d5c7d970..1568e044d 100644 --- a/src/views/task-list-page/config/task-list-workers-table.config.ts +++ b/src/views/task-list-page/config/task-list-workers-table.config.ts @@ -1,7 +1,7 @@ import { createElement } from 'react'; import FormattedDate from '@/components/formatted-date/formatted-date'; -import { type TableColumn } from '@/components/table/table.types'; +import { type TableConfig } from '@/components/table/table.types'; import { type Worker } from '@/route-handlers/describe-task-list/describe-task-list.types'; import TaskListWorkersTableHandlerIcon from '../task-list-workers-table-handler-icon/task-list-workers-table-handler-icon'; From c1da2390fd1dc6680df78701c321dc216e37f5f5 Mon Sep 17 00:00:00 2001 From: Adhitya Mamallan Date: Fri, 22 Nov 2024 16:49:11 +0530 Subject: [PATCH 4/6] Revert changes to table test --- src/components/table/__tests__/table.test.tsx | 54 ++++--------------- 1 file changed, 11 insertions(+), 43 deletions(-) diff --git a/src/components/table/__tests__/table.test.tsx b/src/components/table/__tests__/table.test.tsx index 1131b58f1..c3574e63e 100644 --- a/src/components/table/__tests__/table.test.tsx +++ b/src/components/table/__tests__/table.test.tsx @@ -3,7 +3,6 @@ import React from 'react'; import { render, screen, fireEvent, act } from '@/test-utils/rtl'; import Table from '../table'; -import { type TableConfig } from '../table.types'; type TestDataT = { value: string; @@ -17,49 +16,18 @@ const SAMPLE_ROWS: Array = Array.from( (_, rowIndex) => ({ value: `test_${rowIndex}` }) ); -function getMockRenderCell(index: number) { - return ({ value }: TestDataT) => { - return `data_${value}_${index}`; - }; -} - -const SAMPLE_COLUMNS = [ - { - name: 'Column name 0', - id: 'column_id_0', - sortable: true, - renderCell: getMockRenderCell(0), - width: '20%', - }, - { - name: 'Column name 1', - id: 'column_id_1', - sortable: true, - renderCell: getMockRenderCell(1), - width: '20%', - }, - { - name: 'Column name 2', - id: 'column_id_2', +const SAMPLE_COLUMNS = Array.from( + { length: SAMPLE_DATA_NUM_COLUMNS }, + (_, colIndex) => ({ + name: `Column Name ${colIndex}`, + id: `column_id_${colIndex}`, sortable: true, - renderCell: getMockRenderCell(2), - width: '20%', - }, - { - name: 'Column name 3', - id: 'column_id_3', - sortable: true, - renderCell: getMockRenderCell(3), - width: '20%', - }, - { - name: 'Column name 4', - id: 'column_id_4', - sortable: true, - renderCell: getMockRenderCell(4), - width: '20%', - }, -] as const satisfies TableConfig; + renderCell: ({ value }: TestDataT) => { + return `data_${value}_${colIndex}`; + }, + width: `${100 / SAMPLE_DATA_NUM_COLUMNS}%`, + }) +); describe('Table', () => { it('should render without error', async () => { From 6a8e803c10ba088eb31cd375ed4766e2f871455d Mon Sep 17 00:00:00 2001 From: Adhitya Mamallan Date: Fri, 22 Nov 2024 16:50:15 +0530 Subject: [PATCH 5/6] Remove unnecessary assignment --- .../table/table-sortable-head-cell/table-sortable-head-cell.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/table/table-sortable-head-cell/table-sortable-head-cell.tsx b/src/components/table/table-sortable-head-cell/table-sortable-head-cell.tsx index a64e04952..5a51af7a0 100644 --- a/src/components/table/table-sortable-head-cell/table-sortable-head-cell.tsx +++ b/src/components/table/table-sortable-head-cell/table-sortable-head-cell.tsx @@ -10,7 +10,7 @@ export default function TableSortableHeadCell({ name, columnID, width, - onSort = () => {}, + onSort, sortColumn, sortOrder, }: Props) { From 3a0d48d5beb09684b7261a3404f5663496197310 Mon Sep 17 00:00:00 2001 From: Adhitya Mamallan Date: Fri, 22 Nov 2024 18:17:32 +0530 Subject: [PATCH 6/6] Add tests and fix onSort behaviour --- src/components/table/__tests__/table.test.tsx | 34 +++++++++- .../table-sortable-head-cell.test.tsx | 67 +++++++++++++++++++ .../table-sortable-head-cell.types.ts | 4 +- src/components/table/table.tsx | 11 +-- 4 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 src/components/table/table-sortable-head-cell/__tests__/table-sortable-head-cell.test.tsx diff --git a/src/components/table/__tests__/table.test.tsx b/src/components/table/__tests__/table.test.tsx index c3574e63e..c21f0e643 100644 --- a/src/components/table/__tests__/table.test.tsx +++ b/src/components/table/__tests__/table.test.tsx @@ -11,6 +11,14 @@ type TestDataT = { const SAMPLE_DATA_NUM_ROWS = 10; const SAMPLE_DATA_NUM_COLUMNS = 5; +jest.mock('../table-sortable-head-cell/table-sortable-head-cell', () => + jest.fn(({ name, columnID, onSort }) => ( + onSort(columnID)}> + {name} + + )) +); + const SAMPLE_ROWS: Array = Array.from( { length: SAMPLE_DATA_NUM_ROWS }, (_, rowIndex) => ({ value: `test_${rowIndex}` }) @@ -56,6 +64,11 @@ describe('Table', () => { const { mockOnSort } = setup({ shouldShowResults: true }); const columnElements = await screen.findAllByText(/Column Name \d+/); + expect(columnElements.length).toEqual(5); + + const sortableColumnHeadCells = + await screen.findAllByTestId('sortable-head-cell'); + expect(sortableColumnHeadCells.length).toEqual(5); act(() => { fireEvent.click(columnElements[0]); @@ -63,9 +76,26 @@ describe('Table', () => { expect(mockOnSort).toHaveBeenCalledWith('column_id_0'); }); + + it('should render plain head cells for sortable columns when onSort is missing', async () => { + setup({ shouldShowResults: true, omitOnSort: true }); + + const columnElements = await screen.findAllByText(/Column Name \d+/); + expect(columnElements.length).toEqual(5); + + const sortableColumnHeadCells = + screen.queryAllByTestId('sortable-head-cell'); + expect(sortableColumnHeadCells.length).toEqual(0); + }); }); -function setup({ shouldShowResults }: { shouldShowResults: boolean }) { +function setup({ + shouldShowResults, + omitOnSort, +}: { + shouldShowResults: boolean; + omitOnSort?: boolean; +}) { const mockOnSort = jest.fn(); render( Sample end message} - onSort={mockOnSort} + {...(!omitOnSort && { onSort: mockOnSort })} sortColumn={SAMPLE_COLUMNS[SAMPLE_DATA_NUM_COLUMNS - 1].id} sortOrder="DESC" /> diff --git a/src/components/table/table-sortable-head-cell/__tests__/table-sortable-head-cell.test.tsx b/src/components/table/table-sortable-head-cell/__tests__/table-sortable-head-cell.test.tsx new file mode 100644 index 000000000..355443f45 --- /dev/null +++ b/src/components/table/table-sortable-head-cell/__tests__/table-sortable-head-cell.test.tsx @@ -0,0 +1,67 @@ +import React from 'react'; + +import { render, screen, userEvent } from '@/test-utils/rtl'; + +import { type SortOrder } from '@/utils/sort-by'; + +import TableSortableHeadCell from '../table-sortable-head-cell'; + +describe(TableSortableHeadCell.name, () => { + it('should render unsorted without error', async () => { + setup({ sortColumn: 'column_2', sortOrder: 'DESC' }); + + expect( + await screen.findByLabelText('Column 1, not sorted') + ).toBeInTheDocument(); + }); + + it('should call onSort when clicked', async () => { + const { mockOnSort, user } = setup({ + sortColumn: 'column_2', + sortOrder: 'DESC', + }); + + const cell = await screen.findByLabelText('Column 1, not sorted'); + + await user.click(cell); + expect(mockOnSort).toHaveBeenCalledWith('column_1'); + }); + + it('should render sorted ASC without error', async () => { + setup({ sortColumn: 'column_1', sortOrder: 'ASC' }); + + expect( + await screen.findByLabelText('Column 1, ascending sorting') + ).toBeInTheDocument(); + }); + + it('should render sorted DESC without error', async () => { + setup({ sortColumn: 'column_1', sortOrder: 'DESC' }); + + expect( + await screen.findByLabelText('Column 1, descending sorting') + ).toBeInTheDocument(); + }); +}); + +function setup({ + sortColumn, + sortOrder, +}: { + sortColumn: string; + sortOrder: SortOrder; +}) { + const user = userEvent.setup(); + const mockOnSort = jest.fn(); + render( + + ); + return { mockOnSort, user }; +} diff --git a/src/components/table/table-sortable-head-cell/table-sortable-head-cell.types.ts b/src/components/table/table-sortable-head-cell/table-sortable-head-cell.types.ts index 6024fcfd0..5508fcf25 100644 --- a/src/components/table/table-sortable-head-cell/table-sortable-head-cell.types.ts +++ b/src/components/table/table-sortable-head-cell/table-sortable-head-cell.types.ts @@ -1,8 +1,10 @@ +import { type SortOrder } from '@/utils/sort-by'; + export type Props = { name: string; columnID: string; width: string; sortColumn?: string; - sortOrder?: string; + sortOrder?: SortOrder; onSort: (column: string) => void; }; diff --git a/src/components/table/table.tsx b/src/components/table/table.tsx index 01f2b3cc1..7a8702b7c 100644 --- a/src/components/table/table.tsx +++ b/src/components/table/table.tsx @@ -17,7 +17,9 @@ export default function Table>({ columns, shouldShowResults, endMessage, - ...sortParams + onSort, + sortColumn, + sortOrder, }: Props) { return ( @@ -25,14 +27,15 @@ export default function Table>({ {columns.map((column) => - column.sortable ? ( + column.sortable && typeof onSort === 'function' ? ( {}} - {...sortParams} + onSort={onSort} + sortColumn={sortColumn} + sortOrder={sortOrder} /> ) : (