Skip to content

Commit

Permalink
[Discover] Allow sorting only for visible columns (elastic#172077)
Browse files Browse the repository at this point in the history
- Closes elastic#172023

## Summary

As
[eui](https://github.com/elastic/eui/blob/8eb7277ffd6d13082d6a45030efcf9a2ca0528e8/src/components/datagrid/controls/column_sorting.tsx#L44-L57)
triggers `onSort` for `EuiDataGrid` when `sort` array includes fields
which are not present in `columns` array, it changes the app state. This
results in "unsaved changes" badge although user can't know why.

This PR makes sure that state is not modified when unknown sorting
fields are persisted inside a saved search and user opens such search.

To reproduce the mentioned issue:
- Add columns to the table and define their sorting
- Save the search
- Reopen the search and remove one of the columns which had custom
sorting
- Save the search
- Now open Discover again and reopen the search.

With this PR, "unsaved changes" badge would not appear. 

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
  • Loading branch information
jughosta authored Dec 4, 2023
1 parent 6a71995 commit 7346e82
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 19 deletions.
48 changes: 48 additions & 0 deletions packages/kbn-unified-data-table/src/components/data_table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,54 @@ describe('UnifiedDataTable', () => {
}
`);
});

it('should apply sorting', async () => {
const component = await getComponent({
...getProps(),
sort: [['message', 'desc']],
columns: ['message'],
});

expect(component.find(EuiDataGrid).prop('sorting')).toMatchInlineSnapshot(`
Object {
"columns": Array [
Object {
"direction": "desc",
"id": "message",
},
],
"onSort": [Function],
}
`);
});

it('should not apply unknown sorting', async () => {
const component = await getComponent({
...getProps(),
sort: [
['bytes', 'desc'],
['unknown', 'asc'],
['message', 'desc'],
],
columns: ['bytes', 'message'],
});

expect(component.find(EuiDataGrid).prop('sorting')).toMatchInlineSnapshot(`
Object {
"columns": Array [
Object {
"direction": "desc",
"id": "bytes",
},
Object {
"direction": "desc",
"id": "message",
},
],
"onSort": [Function],
}
`);
});
});

describe('display settings', () => {
Expand Down
45 changes: 26 additions & 19 deletions packages/kbn-unified-data-table/src/components/data_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -538,25 +538,6 @@ export const UnifiedDataTable = ({
);
}, [currentPageSize, setPagination]);

/**
* Sorting
*/
const sortingColumns = useMemo(() => sort.map(([id, direction]) => ({ id, direction })), [sort]);

const [inmemorySortingColumns, setInmemorySortingColumns] = useState([]);
const onTableSort = useCallback(
(sortingColumnsData) => {
if (isSortEnabled) {
if (isPlainRecord) {
setInmemorySortingColumns(sortingColumnsData);
} else if (onSort) {
onSort(sortingColumnsData.map(({ id, direction }: SortObj) => [id, direction]));
}
}
},
[onSort, isSortEnabled, isPlainRecord, setInmemorySortingColumns]
);

const shouldShowFieldHandler = useMemo(() => {
const dataViewFields = dataView.fields.getAll().map((fld) => fld.name);
return getShouldShowFieldHandler(dataViewFields, dataView, showMultiFields);
Expand Down Expand Up @@ -720,6 +701,32 @@ export const UnifiedDataTable = ({
}),
[visibleColumns, hideTimeColumn, onSetColumns]
);

/**
* Sorting
*/
const sortingColumns = useMemo(
() =>
sort
.map(([id, direction]) => ({ id, direction }))
.filter(({ id }) => visibleColumns.includes(id)),
[sort, visibleColumns]
);

const [inmemorySortingColumns, setInmemorySortingColumns] = useState([]);
const onTableSort = useCallback(
(sortingColumnsData) => {
if (isSortEnabled) {
if (isPlainRecord) {
setInmemorySortingColumns(sortingColumnsData);
} else if (onSort) {
onSort(sortingColumnsData.map(({ id, direction }: SortObj) => [id, direction]));
}
}
},
[onSort, isSortEnabled, isPlainRecord, setInmemorySortingColumns]
);

const sorting = useMemo(() => {
if (isSortEnabled) {
return {
Expand Down

0 comments on commit 7346e82

Please sign in to comment.