Skip to content

Commit

Permalink
[EuiInMemoryTable] Fix onTableChange returning the sort field's `na…
Browse files Browse the repository at this point in the history
…me` instead of `field` key (#5588)

* [REVERT] Log onTableChange sort.field output

* Add unit/regression test for buggy behavior

- Should currently fail with field returns as `Name` instead of `name`

* Fix reportedSortName coming back as `name` instead of `field`

* Add changelog entry

* [PR feedback] Dry out findColumnByProp logic

+ fix value typing to accept names which can be ReactNodes

* Revert "[REVERT] Log onTableChange sort.field output"

This reverts commit 3c1e939.

* changelog
  • Loading branch information
Constance authored Feb 2, 2022
1 parent 1d9e30b commit f83d168
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 23 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)

Expand Down
27 changes: 22 additions & 5 deletions src/components/basic_table/in_memory_table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1015,28 +1015,29 @@ describe('EuiInMemoryTable', () => {
};

const component = mount(<EuiInMemoryTable {...props} />);

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,
size: 2,
},
});

(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',
Expand All @@ -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,
},
});
});
});

Expand Down
43 changes: 26 additions & 17 deletions src/components/basic_table/in_memory_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ const getInitialPagination = (pagination: Pagination | undefined) => {
function findColumnByProp<T>(
columns: Array<EuiBasicTableColumn<T>>,
prop: 'field' | 'name',
value: string
value: string | ReactNode
) {
for (let i = 0; i < columns.length; i++) {
const column = columns[i];
Expand All @@ -202,6 +202,19 @@ function findColumnByProp<T>(
}
}

function findColumnByFieldOrName<T>(
columns: Array<EuiBasicTableColumn<T>>,
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<T>(
columns: Array<EuiBasicTableColumn<T>>,
sorting: Sorting | undefined
Expand All @@ -218,12 +231,7 @@ function getInitialSorting<T>(
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 {
Expand Down Expand Up @@ -401,16 +409,17 @@ export class EuiInMemoryTable<T> 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<T>).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 sortColumn = findColumnByFieldOrName(this.props.columns, sortName);
if (sortColumn) {
// Ensure sortName uses `name`
sortName = sortColumn.name as keyof T;

// Ensure reportedSortName uses `field` if it exists
const sortField = (sortColumn as EuiTableFieldDataColumnType<T>).field;
if (sortField) reportedSortName = sortField as keyof T;
}
}

Expand Down

0 comments on commit f83d168

Please sign in to comment.