Skip to content

Commit

Permalink
[Discover] [Unified Data Table] Improve absolute column width handling (
Browse files Browse the repository at this point in the history
#190288)

## Summary

This PR improves the handling of columns with absolute widths in
Discover, including the following enhancements:
- If there are no auto width columns in the profile default app state,
set the last column to auto width so the default columns always fill the
grid.
- If there are no auto width columns remaining when removing a column
from the grid, set the last column to auto width so the remaining
columns fill the grid.
- Add a "Reset width" button to the column header popovers to allow
resetting absolute width columns back to auto width.


https://github.com/user-attachments/assets/0c588969-5720-40e3-91e2-07a83a93b797

Resolves #189817.
Related #188550.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
  • Loading branch information
3 people committed Aug 20, 2024
1 parent 5f19307 commit 349fdac
Show file tree
Hide file tree
Showing 29 changed files with 707 additions and 152 deletions.
4 changes: 2 additions & 2 deletions packages/kbn-unified-data-table/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Props description:
| **dataView** | DataView | The used data view. |
| **loadingState** | DataLoadingState | Determines if data is currently loaded. |
| **onFilter** | DocViewFilterFn | Function to add a filter in the grid cell or document flyout. |
| **onResize** | (optional)(colSettings: { columnId: string; width: number }) => void; | Function triggered when a column is resized by the user. |
| **onResize** | (optional)(colSettings: { columnId: string; width: number | undefind }) => void; | Function triggered when a column is resized by the user, passes `undefined` for auto-width. |
| **onSetColumns** | (columns: string[], hideTimeColumn: boolean) => void; | Function to set all columns. |
| **onSort** | (optional)(sort: string[][]) => void; | Function to change sorting of the documents, skipped when isSortEnabled is set to false. |
| **rows** | (optional)DataTableRecord[] | Array of documents provided by Elasticsearch. |
Expand Down Expand Up @@ -81,7 +81,7 @@ Usage example:
onFilter={() => {
// Add logic to refetch the data when the filter by field was added/removed. Refetch data.
}}
onResize={(colSettings: { columnId: string; width: number }) => {
onResize={(colSettings: { columnId: string; width: number | undefined }) => {
// Update the table state with the new width for the column
}}
onSetColumns={(columns: string[], hideTimeColumn: boolean) => {
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-unified-data-table/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export * as columnActions from './src/components/actions/columns';
export { getRowsPerPageOptions } from './src/utils/rows_per_page';
export { popularizeField } from './src/utils/popularize_field';

export { useColumns } from './src/hooks/use_data_grid_columns';
export { useColumns, type UseColumnsProps } from './src/hooks/use_data_grid_columns';
export { OPEN_DETAILS, SELECT_ROW } from './src/components/data_table_columns'; // TODO: deprecate?
export { DataTableRowControl } from './src/components/data_table_row_control';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ import { getStateColumnActions } from './columns';
import { dataViewMock } from '@kbn/discover-utils/src/__mocks__';
import { Capabilities } from '@kbn/core/types';
import { dataViewsMock } from '../../../__mocks__/data_views';
import { UnifiedDataTableSettings } from '../../types';

function getStateColumnAction(
state: { columns?: string[]; sort?: string[][] },
state: { columns?: string[]; sort?: string[][]; settings?: UnifiedDataTableSettings },
setAppState: (state: { columns: string[]; sort?: string[][] }) => void
) {
return getStateColumnActions({
Expand All @@ -28,6 +29,7 @@ function getStateColumnAction(
columns: state.columns,
sort: state.sort,
defaultOrder: 'desc',
settings: state.settings,
});
}

Expand All @@ -41,6 +43,7 @@ describe('Test column actions', () => {
actions.onAddColumn('test');
expect(setAppState).toHaveBeenCalledWith({ columns: ['test'] });
});

test('getStateColumnActions with columns and sort in state', () => {
const setAppState = jest.fn();
const actions = getStateColumnAction(
Expand Down Expand Up @@ -77,4 +80,95 @@ describe('Test column actions', () => {
columns: ['second', 'first'],
});
});

it('should pass settings to setAppState', () => {
const setAppState = jest.fn();
const settings: UnifiedDataTableSettings = { columns: { first: { width: 100 } } };
const actions = getStateColumnAction({ columns: ['first'], settings }, setAppState);
actions.onAddColumn('second');
expect(setAppState).toHaveBeenCalledWith({ columns: ['first', 'second'], settings });
setAppState.mockClear();
actions.onRemoveColumn('second');
expect(setAppState).toHaveBeenCalledWith({ columns: ['first'], settings, sort: [] });
setAppState.mockClear();
actions.onMoveColumn('first', 0);
expect(setAppState).toHaveBeenCalledWith({ columns: ['first'], settings });
setAppState.mockClear();
actions.onSetColumns(['first', 'second'], true);
expect(setAppState).toHaveBeenCalledWith({ columns: ['first', 'second'], settings });
setAppState.mockClear();
});

it('should clean up settings to remove non-existing columns', () => {
const setAppState = jest.fn();
const actions = getStateColumnAction(
{
columns: ['first', 'second', 'third'],
settings: { columns: { first: { width: 100 }, second: { width: 200 } } },
},
setAppState
);
actions.onRemoveColumn('second');
expect(setAppState).toHaveBeenCalledWith({
columns: ['first', 'third'],
settings: { columns: { first: { width: 100 } } },
sort: [],
});
setAppState.mockClear();
actions.onSetColumns(['first', 'third'], true);
expect(setAppState).toHaveBeenCalledWith({
columns: ['first', 'third'],
settings: { columns: { first: { width: 100 } } },
});
});

it('should reset the last column to auto width if only absolute width columns remain', () => {
const setAppState = jest.fn();
let actions = getStateColumnAction(
{
columns: ['first', 'second', 'third'],
settings: { columns: { second: { width: 100 }, third: { width: 100, display: 'test' } } },
},
setAppState
);
actions.onRemoveColumn('first');
expect(setAppState).toHaveBeenCalledWith({
columns: ['second', 'third'],
settings: { columns: { second: { width: 100 }, third: { display: 'test' } } },
sort: [],
});
setAppState.mockClear();
actions = getStateColumnAction(
{
columns: ['first', 'second', 'third'],
settings: { columns: { second: { width: 100 }, third: { width: 100 } } },
},
setAppState
);
actions.onSetColumns(['second', 'third'], true);
expect(setAppState).toHaveBeenCalledWith({
columns: ['second', 'third'],
settings: { columns: { second: { width: 100 } } },
});
});

it('should not reset the last column to auto width if there are remaining auto width columns', () => {
const setAppState = jest.fn();
const actions = getStateColumnAction(
{ columns: ['first', 'second', 'third'], settings: { columns: { third: { width: 100 } } } },
setAppState
);
actions.onRemoveColumn('first');
expect(setAppState).toHaveBeenCalledWith({
columns: ['second', 'third'],
settings: { columns: { third: { width: 100 } } },
sort: [],
});
setAppState.mockClear();
actions.onSetColumns(['second', 'third'], true);
expect(setAppState).toHaveBeenCalledWith({
columns: ['second', 'third'],
settings: { columns: { third: { width: 100 } } },
});
});
});
172 changes: 124 additions & 48 deletions packages/kbn-unified-data-table/src/components/actions/columns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,52 +5,13 @@
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
import { Capabilities } from '@kbn/core/public';

import type { Capabilities } from '@kbn/core/public';
import type { DataViewsContract } from '@kbn/data-plugin/public';
import type { DataView } from '@kbn/data-views-plugin/public';
import { omit } from 'lodash';
import { popularizeField } from '../../utils/popularize_field';

/**
* Helper function to provide a fallback to a single _source column if the given array of columns
* is empty, and removes _source if there are more than 1 columns given
* @param columns
* @param useNewFieldsApi should a new fields API be used
*/
function buildColumns(columns: string[], useNewFieldsApi = false) {
if (columns.length > 1 && columns.indexOf('_source') !== -1) {
return columns.filter((col) => col !== '_source');
} else if (columns.length !== 0) {
return columns;
}
return useNewFieldsApi ? [] : ['_source'];
}

export function addColumn(columns: string[], columnName: string, useNewFieldsApi?: boolean) {
if (columns.includes(columnName)) {
return columns;
}
return buildColumns([...columns, columnName], useNewFieldsApi);
}

export function removeColumn(columns: string[], columnName: string, useNewFieldsApi?: boolean) {
if (!columns.includes(columnName)) {
return columns;
}
return buildColumns(
columns.filter((col) => col !== columnName),
useNewFieldsApi
);
}

export function moveColumn(columns: string[], columnName: string, newIndex: number) {
if (newIndex < 0 || newIndex >= columns.length || !columns.includes(columnName)) {
return columns;
}
const modifiedColumns = [...columns];
modifiedColumns.splice(modifiedColumns.indexOf(columnName), 1); // remove at old index
modifiedColumns.splice(newIndex, 0, columnName); // insert before new index
return modifiedColumns;
}
import type { UnifiedDataTableSettings } from '../../types';

export function getStateColumnActions({
capabilities,
Expand All @@ -61,34 +22,50 @@ export function getStateColumnActions({
columns,
sort,
defaultOrder,
settings,
}: {
capabilities: Capabilities;
dataView: DataView;
dataViews: DataViewsContract;
useNewFieldsApi: boolean;
setAppState: (state: { columns: string[]; sort?: string[][] }) => void;
setAppState: (state: {
columns: string[];
sort?: string[][];
settings?: UnifiedDataTableSettings;
}) => void;
columns?: string[];
sort: string[][] | undefined;
defaultOrder: string;
settings?: UnifiedDataTableSettings;
}) {
function onAddColumn(columnName: string) {
popularizeField(dataView, columnName, dataViews, capabilities);
const nextColumns = addColumn(columns || [], columnName, useNewFieldsApi);
const nextSort = columnName === '_score' && !sort?.length ? [['_score', defaultOrder]] : sort;
setAppState({ columns: nextColumns, sort: nextSort });
setAppState({ columns: nextColumns, sort: nextSort, settings });
}

function onRemoveColumn(columnName: string) {
popularizeField(dataView, columnName, dataViews, capabilities);

const nextColumns = removeColumn(columns || [], columnName, useNewFieldsApi);
// The state's sort property is an array of [sortByColumn,sortDirection]
const nextSort = sort && sort.length ? sort.filter((subArr) => subArr[0] !== columnName) : [];
setAppState({ columns: nextColumns, sort: nextSort });

let nextSettings = cleanColumnSettings(nextColumns, settings);

// When columns are removed, reset the last column to auto width if only absolute
// width columns remain, to ensure the columns fill the available grid space
if (nextColumns.length < (columns?.length ?? 0)) {
nextSettings = adjustLastColumnWidth(nextColumns, nextSettings);
}

setAppState({ columns: nextColumns, sort: nextSort, settings: nextSettings });
}

function onMoveColumn(columnName: string, newIndex: number) {
const nextColumns = moveColumn(columns || [], columnName, newIndex);
setAppState({ columns: nextColumns });
setAppState({ columns: nextColumns, settings });
}

function onSetColumns(nextColumns: string[], hideTimeColumn: boolean) {
Expand All @@ -98,7 +75,15 @@ export function getStateColumnActions({
? (nextColumns || []).slice(1)
: nextColumns;

setAppState({ columns: actualColumns });
let nextSettings = cleanColumnSettings(nextColumns, settings);

// When columns are removed, reset the last column to auto width if only absolute
// width columns remain, to ensure the columns fill the available grid space
if (actualColumns.length < (columns?.length ?? 0)) {
nextSettings = adjustLastColumnWidth(actualColumns, nextSettings);
}

setAppState({ columns: actualColumns, settings: nextSettings });
}
return {
onAddColumn,
Expand All @@ -107,3 +92,94 @@ export function getStateColumnActions({
onSetColumns,
};
}

/**
* Helper function to provide a fallback to a single _source column if the given array of columns
* is empty, and removes _source if there are more than 1 columns given
* @param columns
* @param useNewFieldsApi should a new fields API be used
*/
function buildColumns(columns: string[], useNewFieldsApi = false) {
if (columns.length > 1 && columns.indexOf('_source') !== -1) {
return columns.filter((col) => col !== '_source');
} else if (columns.length !== 0) {
return columns;
}
return useNewFieldsApi ? [] : ['_source'];
}

function addColumn(columns: string[], columnName: string, useNewFieldsApi?: boolean) {
if (columns.includes(columnName)) {
return columns;
}
return buildColumns([...columns, columnName], useNewFieldsApi);
}

function removeColumn(columns: string[], columnName: string, useNewFieldsApi?: boolean) {
if (!columns.includes(columnName)) {
return columns;
}
return buildColumns(
columns.filter((col) => col !== columnName),
useNewFieldsApi
);
}

function moveColumn(columns: string[], columnName: string, newIndex: number) {
if (newIndex < 0 || newIndex >= columns.length || !columns.includes(columnName)) {
return columns;
}
const modifiedColumns = [...columns];
modifiedColumns.splice(modifiedColumns.indexOf(columnName), 1); // remove at old index
modifiedColumns.splice(newIndex, 0, columnName); // insert before new index
return modifiedColumns;
}

function cleanColumnSettings(
columns: string[],
settings?: UnifiedDataTableSettings
): UnifiedDataTableSettings | undefined {
const columnSettings = settings?.columns;

if (!columnSettings) {
return settings;
}

const nextColumnSettings = columns.reduce<NonNullable<UnifiedDataTableSettings['columns']>>(
(acc, column) => (columnSettings[column] ? { ...acc, [column]: columnSettings[column] } : acc),
{}
);

return { ...settings, columns: nextColumnSettings };
}

function adjustLastColumnWidth(
columns: string[],
settings?: UnifiedDataTableSettings
): UnifiedDataTableSettings | undefined {
const columnSettings = settings?.columns;

if (!columns.length || !columnSettings) {
return settings;
}

const hasAutoWidthColumn = columns.some((colId) => columnSettings[colId]?.width == null);

if (hasAutoWidthColumn) {
return settings;
}

const lastColumn = columns[columns.length - 1];
const lastColumnSettings = omit(columnSettings[lastColumn] ?? {}, 'width');
const lastColumnSettingsOptional = Object.keys(lastColumnSettings).length
? { [lastColumn]: lastColumnSettings }
: undefined;

return {
...settings,
columns: {
...omit(columnSettings, lastColumn),
...lastColumnSettingsOptional,
},
};
}
Loading

0 comments on commit 349fdac

Please sign in to comment.