From 3c1e93920cabbf6b6bd7bca5f57520ef9a6ada23 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 1 Feb 2022 11:24:39 -0800 Subject: [PATCH 1/7] [REVERT] Log onTableChange sort.field output --- src-docs/src/views/tables/in_memory/in_memory.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src-docs/src/views/tables/in_memory/in_memory.js b/src-docs/src/views/tables/in_memory/in_memory.js index 65d8c0a8712..52390cfd1f3 100644 --- a/src-docs/src/views/tables/in_memory/in_memory.js +++ b/src-docs/src/views/tables/in_memory/in_memory.js @@ -95,6 +95,7 @@ export const Table = () => { columns={columns} pagination={true} sorting={sorting} + onTableChange={(values) => console.log(values.sort)} /> ); }; From c8938fa36f418f555da091f1597e53a633237cf5 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 1 Feb 2022 11:29:04 -0800 Subject: [PATCH 2/7] Add unit/regression test for buggy behavior - Should currently fail with field returns as `Name` instead of `name` --- .../basic_table/in_memory_table.test.tsx | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/components/basic_table/in_memory_table.test.tsx b/src/components/basic_table/in_memory_table.test.tsx index b6cc969fec2..a768338c2fa 100644 --- a/src/components/basic_table/in_memory_table.test.tsx +++ b/src/components/basic_table/in_memory_table.test.tsx @@ -1015,13 +1015,14 @@ describe('EuiInMemoryTable', () => { }; const component = mount(); - expect(props.onTableChange).toHaveBeenCalledTimes(0); + + // Pagination change component .find('EuiButtonEmpty[data-test-subj="pagination-button-1"]') .simulate('click'); expect(props.onTableChange).toHaveBeenCalledTimes(1); - expect(props.onTableChange).toHaveBeenCalledWith({ + expect(props.onTableChange).toHaveBeenLastCalledWith({ sort: {}, page: { index: 1, @@ -1029,14 +1030,14 @@ describe('EuiInMemoryTable', () => { }, }); - (props.onTableChange as jest.Mock).mockClear(); + // Sorting change component .find( '[data-test-subj*="tableHeaderCell_name_0"] [data-test-subj="tableHeaderSortButton"]' ) .simulate('click'); - expect(props.onTableChange).toHaveBeenCalledTimes(1); - expect(props.onTableChange).toHaveBeenCalledWith({ + expect(props.onTableChange).toHaveBeenCalledTimes(2); + expect(props.onTableChange).toHaveBeenLastCalledWith({ sort: { direction: SortDirection.ASC, field: 'name', @@ -1046,6 +1047,22 @@ describe('EuiInMemoryTable', () => { size: 2, }, }); + + // Sorted pagination change + component + .find('EuiButtonEmpty[data-test-subj="pagination-button-1"]') + .simulate('click'); + expect(props.onTableChange).toHaveBeenCalledTimes(3); + expect(props.onTableChange).toHaveBeenLastCalledWith({ + sort: { + direction: SortDirection.ASC, + field: 'name', + }, + page: { + index: 1, + size: 2, + }, + }); }); }); From d15526020445f7de44a2ef471198ebd9dfbdea0b Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 1 Feb 2022 11:31:17 -0800 Subject: [PATCH 3/7] Fix reportedSortName coming back as `name` instead of `field` --- .../basic_table/in_memory_table.tsx | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/components/basic_table/in_memory_table.tsx b/src/components/basic_table/in_memory_table.tsx index 0509cc78752..78774183aa6 100644 --- a/src/components/basic_table/in_memory_table.tsx +++ b/src/components/basic_table/in_memory_table.tsx @@ -401,16 +401,21 @@ export class EuiInMemoryTable extends Component< // from sortName; sortName gets stored internally while reportedSortName is sent to the callback let reportedSortName = sortName; - // EuiBasicTable returns the column's `field` if it exists instead of `name`, - // map back to `name` if this is the case - for (let i = 0; i < this.props.columns.length; i++) { - const column = this.props.columns[i]; - if ( - 'field' in column && - (column as EuiTableFieldDataColumnType).field === sortName - ) { - sortName = column.name as keyof T; - break; + // EuiBasicTable returns the column's `field` instead of `name` on sort + // and the column's `name` instead of `field` on pagination + if (sortName) { + const { columns } = this.props; + let sortColumn = findColumnByProp(columns, 'field', sortName as string); + if (sortColumn == null) { + sortColumn = findColumnByProp(columns, 'name', sortName as string); + } + if (sortColumn) { + // Ensure sortName uses `name` + sortName = sortColumn.name as keyof T; + + // Ensure reportedSortName uses `field` if it exists + const sortField = (sortColumn as EuiTableFieldDataColumnType).field; + if (sortField) reportedSortName = sortField as keyof T; } } From e1cee95f657a2b0c9cbb2a36d672cd46914f4f68 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 1 Feb 2022 11:49:07 -0800 Subject: [PATCH 4/7] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fce69960dc..40b464beea5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ - Fixed a focus bug in `EuiDataGrid` when clicking another cell header with an already-open cell header popover ([#5556](https://github.com/elastic/eui/pull/5556)) - Fixed `EuiDataGrid` to always focus back into the grid on pagination ([#5587](https://github.com/elastic/eui/pull/5587)) - Fixed `EuiDataGrid` and `EuiTable` pagination potentially rendering out view on narrow tables with many pages ([#5561](https://github.com/elastic/eui/pull/5561)) +- Fixed `EuiInMemoryTable`'s `onTableChange` callback not returning the correct `sort.field` value on pagination ([#5588](https://github.com/elastic/eui/pull/5588)) ## [`46.1.0`](https://github.com/elastic/eui/tree/v46.1.0) From 16dc23433b77fb104c7e66daaf9e8bb792c4bdaa Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 1 Feb 2022 12:34:08 -0800 Subject: [PATCH 5/7] [PR feedback] Dry out findColumnByProp logic + fix value typing to accept names which can be ReactNodes --- .../basic_table/in_memory_table.tsx | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/components/basic_table/in_memory_table.tsx b/src/components/basic_table/in_memory_table.tsx index 78774183aa6..548840a2e9a 100644 --- a/src/components/basic_table/in_memory_table.tsx +++ b/src/components/basic_table/in_memory_table.tsx @@ -188,7 +188,7 @@ const getInitialPagination = (pagination: Pagination | undefined) => { function findColumnByProp( columns: Array>, prop: 'field' | 'name', - value: string + value: string | ReactNode ) { for (let i = 0; i < columns.length; i++) { const column = columns[i]; @@ -202,6 +202,19 @@ function findColumnByProp( } } +function findColumnByFieldOrName( + columns: Array>, + value: string | ReactNode +) { + // The passed value can be a column's `field` or its `name` + // for backwards compatibility `field` must be checked first + let column = findColumnByProp(columns, 'field', value); + if (column == null) { + column = findColumnByProp(columns, 'name', value); + } + return column; +} + function getInitialSorting( columns: Array>, sorting: Sorting | undefined @@ -218,12 +231,7 @@ function getInitialSorting( direction: sortDirection, } = (sorting as SortingOptions).sort; - // sortable could be a column's `field` or its `name` - // for backwards compatibility `field` must be checked first - let sortColumn = findColumnByProp(columns, 'field', sortable); - if (sortColumn == null) { - sortColumn = findColumnByProp(columns, 'name', sortable); - } + const sortColumn = findColumnByFieldOrName(columns, sortable); if (sortColumn == null) { return { @@ -404,11 +412,7 @@ export class EuiInMemoryTable extends Component< // EuiBasicTable returns the column's `field` instead of `name` on sort // and the column's `name` instead of `field` on pagination if (sortName) { - const { columns } = this.props; - let sortColumn = findColumnByProp(columns, 'field', sortName as string); - if (sortColumn == null) { - sortColumn = findColumnByProp(columns, 'name', sortName as string); - } + const sortColumn = findColumnByFieldOrName(this.props.columns, sortName); if (sortColumn) { // Ensure sortName uses `name` sortName = sortColumn.name as keyof T; From 084b8e452318ae0ab0e92fcbc354952af117c57a Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 1 Feb 2022 12:36:37 -0800 Subject: [PATCH 6/7] Revert "[REVERT] Log onTableChange sort.field output" This reverts commit 3c1e93920cabbf6b6bd7bca5f57520ef9a6ada23. --- src-docs/src/views/tables/in_memory/in_memory.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src-docs/src/views/tables/in_memory/in_memory.js b/src-docs/src/views/tables/in_memory/in_memory.js index 52390cfd1f3..65d8c0a8712 100644 --- a/src-docs/src/views/tables/in_memory/in_memory.js +++ b/src-docs/src/views/tables/in_memory/in_memory.js @@ -95,7 +95,6 @@ export const Table = () => { columns={columns} pagination={true} sorting={sorting} - onTableChange={(values) => console.log(values.sort)} /> ); }; From f3bc8f66559c31f45a0f2aea1e1eee36bc0e1c55 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 1 Feb 2022 15:11:26 -0800 Subject: [PATCH 7/7] changelog --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da2322b183a..99a907b60bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## [`main`](https://github.com/elastic/eui/tree/main) -No public interface changes since `46.2.0`. +**Bug fixes** + +- Fixed `EuiInMemoryTable`'s `onTableChange` callback not returning the correct `sort.field` value on pagination ([#5588](https://github.com/elastic/eui/pull/5588)) ## [`46.2.0`](https://github.com/elastic/eui/tree/v46.2.0) @@ -19,7 +21,6 @@ No public interface changes since `46.2.0`. - Fixed a focus bug in `EuiDataGrid` when clicking another cell header with an already-open cell header popover ([#5556](https://github.com/elastic/eui/pull/5556)) - Fixed `EuiDataGrid` to always focus back into the grid on pagination ([#5587](https://github.com/elastic/eui/pull/5587)) - Fixed `EuiDataGrid` and `EuiTable` pagination potentially rendering out view on narrow tables with many pages ([#5561](https://github.com/elastic/eui/pull/5561)) -- Fixed `EuiInMemoryTable`'s `onTableChange` callback not returning the correct `sort.field` value on pagination ([#5588](https://github.com/elastic/eui/pull/5588)) ## [`46.1.0`](https://github.com/elastic/eui/tree/v46.1.0)