Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover] [Unified Data Table] Improve absolute column width handling #190288

Merged
merged 15 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 114 additions & 34 deletions packages/kbn-unified-data-table/src/components/data_table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
import React from 'react';
import React, { useState } from 'react';
import { ReactWrapper } from 'enzyme';
import {
EuiButton,
Expand Down Expand Up @@ -33,6 +33,7 @@ import { DatatableColumnType } from '@kbn/expressions-plugin/common';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { CELL_CLASS } from '../utils/get_render_cell_value';
import { defaultTimeColumnWidth } from '../constants';

const mockUseDataGridColumnsCellActions = jest.fn((prop: unknown) => []);
jest.mock('@kbn/cell-actions', () => ({
Expand Down Expand Up @@ -90,6 +91,37 @@ const DataTable = (props: Partial<UnifiedDataTableProps>) => (
</KibanaContextProvider>
);

const renderDataTable = (props: Partial<UnifiedDataTableProps>) => {
const DataTableWrapped = () => {
const [columns, setColumns] = useState(props.columns ?? []);
const [settings, setSettings] = useState(props.settings);

return (
<IntlProvider locale="en">
<DataTable
{...props}
columns={columns}
onSetColumns={setColumns}
settings={settings}
onResize={({ columnId, width }) => {
setSettings({
...settings,
columns: {
...settings?.columns,
[columnId]: {
width,
},
},
});
}}
/>
</IntlProvider>
);
};

render(<DataTableWrapped />);
};

async function getComponent(props: UnifiedDataTableProps = getProps()) {
const component = mountWithIntl(<DataTable {...props} />);
await act(async () => {
Expand Down Expand Up @@ -265,34 +297,19 @@ describe('UnifiedDataTable', () => {

describe('edit field button', () => {
it('should render the edit field button if onFieldEdited is provided', async () => {
const component = await getComponent({
...getProps(),
columns: ['message'],
onFieldEdited: jest.fn(),
});
expect(findTestSubject(component, 'dataGridHeaderCellActionGroup-message').exists()).toBe(
false
);
findTestSubject(component, 'dataGridHeaderCell-message').find('button').simulate('click');
expect(findTestSubject(component, 'dataGridHeaderCellActionGroup-message').exists()).toBe(
true
);
expect(findTestSubject(component, 'gridEditFieldButton').exists()).toBe(true);
renderDataTable({ columns: ['message'], onFieldEdited: jest.fn() });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to rewrite these tests in RTL because Enzyme doesn't properly clean up portals after tests (used for the column header popovers), causing my new tests below to fail.

expect(screen.queryByTestId('dataGridHeaderCellActionGroup-message')).not.toBeInTheDocument();
userEvent.click(screen.getByRole('button', { name: 'message' }));
expect(screen.getByTestId('dataGridHeaderCellActionGroup-message')).toBeInTheDocument();
expect(screen.getByTestId('gridEditFieldButton')).toBeInTheDocument();
});

it('should not render the edit field button if onFieldEdited is not provided', async () => {
const component = await getComponent({
...getProps(),
columns: ['message'],
});
expect(findTestSubject(component, 'dataGridHeaderCellActionGroup-message').exists()).toBe(
false
);
findTestSubject(component, 'dataGridHeaderCell-message').find('button').simulate('click');
expect(findTestSubject(component, 'dataGridHeaderCellActionGroup-message').exists()).toBe(
true
);
expect(findTestSubject(component, 'gridEditFieldButton').exists()).toBe(false);
renderDataTable({ columns: ['message'] });
expect(screen.queryByTestId('dataGridHeaderCellActionGroup-message')).not.toBeInTheDocument();
userEvent.click(screen.getByRole('button', { name: 'message' }));
expect(screen.getByTestId('dataGridHeaderCellActionGroup-message')).toBeInTheDocument();
expect(screen.queryByTestId('gridEditFieldButton')).not.toBeInTheDocument();
});
});

Expand Down Expand Up @@ -789,14 +806,6 @@ describe('UnifiedDataTable', () => {
});

describe('document comparison', () => {
const renderDataTable = (props: Partial<UnifiedDataTableProps>) => {
render(
<IntlProvider locale="en">
<DataTable {...props} />
</IntlProvider>
);
};

const getSelectedDocumentsButton = () => screen.queryByTestId('unifiedDataTableSelectionBtn');

const selectDocument = (document: EsHitRecord) =>
Expand Down Expand Up @@ -916,4 +925,75 @@ describe('UnifiedDataTable', () => {
expect(findTestSubject(component, 'dataGridHeaderCell-colorIndicator').exists()).toBeFalsy();
});
});

describe('columns', () => {
// Default column width in EUI is hardcoded to 100px for Jest envs
const EUI_DEFAULT_COLUMN_WIDTH = '100px';
const getColumnHeader = (name: string) => screen.getByRole('columnheader', { name });
const queryColumnHeader = (name: string) => screen.queryByRole('columnheader', { name });
const getButton = (name: string) => screen.getByRole('button', { name });
const queryButton = (name: string) => screen.queryByRole('button', { name });

it('should reset the last column to auto width when removing the only auto width column', async () => {
renderDataTable({
columns: ['message', 'extension', 'bytes'],
settings: {
columns: {
extension: { width: 50 },
bytes: { width: 50 },
},
},
});
expect(getColumnHeader('message')).toHaveStyle({ width: EUI_DEFAULT_COLUMN_WIDTH });
expect(getColumnHeader('extension')).toHaveStyle({ width: '50px' });
expect(getColumnHeader('bytes')).toHaveStyle({ width: '50px' });
userEvent.click(getButton('message'));
userEvent.click(getButton('Remove column'), undefined, { skipPointerEventsCheck: true });
expect(queryColumnHeader('message')).not.toBeInTheDocument();
expect(getColumnHeader('extension')).toHaveStyle({ width: '50px' });
expect(getColumnHeader('bytes')).toHaveStyle({ width: EUI_DEFAULT_COLUMN_WIDTH });
});

it('should not reset the last column to auto width when there are multiple auto width columns', async () => {
renderDataTable({
columns: ['message', 'extension', 'bytes'],
settings: {
columns: {
bytes: { width: 50 },
},
},
});
expect(getColumnHeader('message')).toHaveStyle({ width: EUI_DEFAULT_COLUMN_WIDTH });
expect(getColumnHeader('extension')).toHaveStyle({ width: EUI_DEFAULT_COLUMN_WIDTH });
expect(getColumnHeader('bytes')).toHaveStyle({ width: '50px' });
userEvent.click(getButton('message'));
userEvent.click(getButton('Remove column'), undefined, { skipPointerEventsCheck: true });
expect(queryColumnHeader('message')).not.toBeInTheDocument();
expect(getColumnHeader('extension')).toHaveStyle({ width: EUI_DEFAULT_COLUMN_WIDTH });
expect(getColumnHeader('bytes')).toHaveStyle({ width: '50px' });
});

it('should show the reset width button only for absolute width columns, and allow resetting to default width', async () => {
renderDataTable({
columns: ['message', 'extension'],
settings: {
columns: {
'@timestamp': { width: 50 },
extension: { width: 50 },
},
},
});
expect(getColumnHeader('@timestamp')).toHaveStyle({ width: '50px' });
userEvent.click(getButton('@timestamp'));
userEvent.click(getButton('Reset width'), undefined, { skipPointerEventsCheck: true });
expect(getColumnHeader('@timestamp')).toHaveStyle({ width: `${defaultTimeColumnWidth}px` });
expect(getColumnHeader('message')).toHaveStyle({ width: EUI_DEFAULT_COLUMN_WIDTH });
userEvent.click(getButton('message'));
expect(queryButton('Reset width')).not.toBeInTheDocument();
expect(getColumnHeader('extension')).toHaveStyle({ width: '50px' });
userEvent.click(getButton('extension'));
userEvent.click(getButton('Reset width'), undefined, { skipPointerEventsCheck: true });
expect(getColumnHeader('extension')).toHaveStyle({ width: EUI_DEFAULT_COLUMN_WIDTH });
});
});
});
25 changes: 24 additions & 1 deletion packages/kbn-unified-data-table/src/components/data_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import type { ThemeServiceStart } from '@kbn/react-kibana-context-common';
import type { DataPublicPluginStart } from '@kbn/data-plugin/public';
import type { DocViewFilterFn } from '@kbn/unified-doc-viewer/types';
import { AdditionalFieldGroups } from '@kbn/unified-field-list';
import { isEqual } from 'lodash';
import {
UnifiedDataTableSettings,
ValueToStringConverter,
Expand Down Expand Up @@ -163,7 +164,7 @@ export interface UnifiedDataTableProps {
/**
* Function triggered when a column is resized by the user
*/
onResize?: (colSettings: { columnId: string; width: number }) => void;
onResize?: (colSettings: { columnId: string; width?: number }) => void;
/**
* Function to set all columns
*/
Expand Down Expand Up @@ -778,6 +779,7 @@ export const UnifiedDataTable = ({
showColumnTokens,
headerRowHeightLines,
customGridColumnsConfiguration,
onResize,
}),
[
columnsMeta,
Expand All @@ -792,6 +794,7 @@ export const UnifiedDataTable = ({
isPlainRecord,
isSortEnabled,
onFilter,
onResize,
settings,
showColumnTokens,
toastNotifications,
Expand All @@ -814,6 +817,26 @@ export const UnifiedDataTable = ({
[visibleColumns, onSetColumns, shouldPrependTimeFieldColumn]
);

// When columns are modified, reset the last column to auto width if only absolute
// width columns remain, to ensure the columns fill the available grid space
const prevColumns = useRef<string[]>();

useEffect(() => {
if (isEqual(prevColumns.current, visibleColumns)) {
return;
}

prevColumns.current = visibleColumns;

const hasAutoWidthColumn = visibleColumns.some(
(colId) => euiGridColumns.find((col) => col.id === colId)?.initialWidth == null
);

if (!hasAutoWidthColumn) {
onResize?.({ columnId: visibleColumns[visibleColumns.length - 1] });
}
}, [euiGridColumns, onResize, visibleColumns]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to save a search with all fixed width columns. But once saved/reloaded, the last column got reset and "Unsaved changes" badge appeared. Is it because of this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this turned out to be a pretty naive implementation. I tested it more thoroughly and ran into several edge cases where it would fail. I updated to a more robust approach here, which also allows consumers to have more control over the behaviour: 7b3583e. I'll need to update tests tomorrow for the new approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests updated: 4cf2b41

/**
* Sorting
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ describe('Data table columns', function () {
hasEditDataViewPermission: () =>
servicesMock.dataViewFieldEditor.userPermissions.editIndexPattern(),
onFilter: () => {},
onResize: () => {},
});
expect(actual).toMatchSnapshot();
});
Expand All @@ -72,6 +73,7 @@ describe('Data table columns', function () {
hasEditDataViewPermission: () =>
servicesMock.dataViewFieldEditor.userPermissions.editIndexPattern(),
onFilter: () => {},
onResize: () => {},
});
expect(actual).toMatchSnapshot();
});
Expand Down Expand Up @@ -99,6 +101,7 @@ describe('Data table columns', function () {
message: { type: 'string', esType: 'keyword' },
timestamp: { type: 'date', esType: 'dateTime' },
},
onResize: () => {},
});
expect(actual).toMatchSnapshot();
});
Expand Down Expand Up @@ -297,6 +300,7 @@ describe('Data table columns', function () {
hasEditDataViewPermission: () =>
servicesMock.dataViewFieldEditor.userPermissions.editIndexPattern(),
onFilter: () => {},
onResize: () => {},
});
expect(actual).toMatchSnapshot();
});
Expand Down Expand Up @@ -324,6 +328,7 @@ describe('Data table columns', function () {
hasEditDataViewPermission: () =>
servicesMock.dataViewFieldEditor.userPermissions.editIndexPattern(),
onFilter: () => {},
onResize: () => {},
});
expect(actual).toMatchSnapshot();
});
Expand Down Expand Up @@ -356,6 +361,7 @@ describe('Data table columns', function () {
columnsMeta: {
extension: { type: 'string' },
},
onResize: () => {},
});
expect(gridColumns[1].schema).toBe('string');
});
Expand Down Expand Up @@ -386,6 +392,7 @@ describe('Data table columns', function () {
columnsMeta: {
var_test: { type: 'number' },
},
onResize: () => {},
});
expect(gridColumns[1].schema).toBe('numeric');
});
Expand All @@ -412,6 +419,7 @@ describe('Data table columns', function () {
extension: { type: 'string' },
message: { type: 'string', esType: 'keyword' },
},
onResize: () => {},
});

const extensionGridColumn = gridColumns[0];
Expand Down Expand Up @@ -442,6 +450,7 @@ describe('Data table columns', function () {
extension: { type: 'string' },
message: { type: 'string', esType: 'keyword' },
},
onResize: () => {},
});

expect(customizedGridColumns).toMatchSnapshot();
Expand Down Expand Up @@ -484,6 +493,7 @@ describe('Data table columns', function () {
},
hasEditDataViewPermission: () =>
servicesMock.dataViewFieldEditor.userPermissions.editIndexPattern(),
onResize: () => {},
});
const columnDisplayNames = customizedGridColumns.map((column) => column.displayAsText);
expect(columnDisplayNames.includes('test_column_one')).toBeTruthy();
Expand Down
Loading