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

davismcphee · 2024-08-12T00:15:08Z

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.

absolute_width.mp4

Resolves #189817.
Related #188550.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

davismcphee · 2024-08-12T00:15:16Z

/ci

… least one auto width column by default

… width column is removed

davismcphee · 2024-08-12T22:17:41Z

/ci

davismcphee · 2024-08-13T04:34:04Z

/ci

davismcphee · 2024-08-13T21:51:11Z

packages/kbn-unified-data-table/src/components/data_table.test.tsx

-        true
-      );
-      expect(findTestSubject(component, 'gridEditFieldButton').exists()).toBe(true);
+      renderDataTable({ columns: ['message'], onFieldEdited: jest.fn() });


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.

davismcphee · 2024-08-13T21:51:19Z

/ci

elasticmachine · 2024-08-14T01:33:20Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jughosta

Nice! Thanks for implementing the column width reset!

jughosta · 2024-08-14T15:04:29Z

src/plugins/discover/public/utils/on_resize_grid_column.ts

@@ -9,13 +9,13 @@
 import type { DiscoverGridSettings } from '@kbn/saved-search-plugin/common';

 export const onResizeGridColumn = (
-  colSettings: { columnId: string; width: number },
+  colSettings: { columnId: string; width?: number },


nit to make it more explicit that the width is not missing rather needs to be reset.
We could also leave a comment explaining the logic.

Suggested change

colSettings: { columnId: string; width?: number },

colSettings: { columnId: string; width: number | undefined },

Agreed 👍 Updated here and added comments explaining why it can be undefined: 9844535

jughosta · 2024-08-14T15:25:17Z

packages/kbn-unified-data-table/src/components/data_table.tsx

+  // 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]);
+


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?

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.

Tests updated: 4cf2b41

jughosta · 2024-08-14T15:31:46Z

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

@@ -179,7 +180,7 @@ function DiscoverDocumentsComponent({
    [stateContainer]
  );

-  const onResizeDataGrid = useCallback(
+  const onResizeDataGrid = useCallback<NonNullable<UnifiedDataTableProps['onResize']>>(


I also checked Dashboard page and it does not seem to offer resetting a column width. Should it?

That was because the saved search embeddable didn't support onResize or grid modifications, but I added support for it now: 7b3583e.

…olumn resizing to surrounding docs and saved search panels

…fix'

opauloh

CSP posture changes LGTM!

jughosta

Works great, thanks! 👍

michaelolo24

Thank you for making the changes in security solution as well. Works wonderfully!

kibana-ci · 2024-08-20T19:53:29Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: c655977

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/unified-data-table`	94	108	+14

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`cloudSecurityPosture`	479.3KB	480.4KB	+1.1KB
`discover`	844.1KB	843.6KB	-524.0B
`esqlDataGrid`	129.1KB	129.4KB	+345.0B
`securitySolution`	20.7MB	20.7MB	+3.9KB
`slo`	915.5KB	915.9KB	+345.0B
total			+5.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	47.7KB	47.7KB	+7.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/unified-data-table`	170	184	+14

History

💛 Build #228111 was flaky 489c502
💛 Build #228050 was flaky 6c32514
💔 Build #227984 failed 641eee3
💔 Build #227813 failed 7b3583e
💚 Build #227510 succeeded ad227b5
💛 Build #227498 was flaky ad227b5

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @davismcphee

davismcphee self-assigned this Aug 12, 2024

davismcphee added 4 commits August 12, 2024 19:17

Skip default profile columns for empty fields, and ensure there is at…

94fbbb5

… least one auto width column by default

Add reset width button and auto resize last column when the last auto…

406528a

… width column is removed

Revert changes to hide default profile columns with empty values

13c52f1

Cleanup and Jest test updates

2603c14

davismcphee force-pushed the empty-default-columns branch from 1ea3a8c to 2603c14 Compare August 12, 2024 22:17

Update last column auto width logic and add Jest tests

0c656e4

Add more Jest tests

ad227b5

davismcphee commented Aug 13, 2024

View reviewed changes

davismcphee changed the title ~~[Discover] Hide default profile columns without values~~ [Discover] [Unified Data Table] Improve absolute column width handling Aug 13, 2024

davismcphee added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 13, 2024

davismcphee marked this pull request as ready for review August 14, 2024 01:33

davismcphee requested review from a team as code owners August 14, 2024 01:33

jughosta reviewed Aug 14, 2024

View reviewed changes

davismcphee added 4 commits August 15, 2024 01:24

Specify width as number | undefined

9844535

Update approach to resetting last column width, and add support for c…

7b3583e

…olumn resizing to surrounding docs and saved search panels

Merge branch 'main' into empty-default-columns

641eee3

Add remaining tests

4cf2b41

davismcphee and others added 2 commits August 15, 2024 18:26

Merge branch 'main' into empty-default-columns

5b8a656

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

6c32514

…fix'

opauloh approved these changes Aug 15, 2024

View reviewed changes

Merge branch 'main' into empty-default-columns

489c502

jughosta approved these changes Aug 19, 2024

View reviewed changes

michaelolo24 approved these changes Aug 20, 2024

View reviewed changes

davismcphee added 2 commits August 20, 2024 15:25

Merge branch 'main' into empty-default-columns

0857c7b

Fix merge conflict

c655977

davismcphee merged commit 349fdac into elastic:main Aug 20, 2024
40 checks passed

davismcphee deleted the empty-default-columns branch August 20, 2024 20:27

kibanamachine added the v8.16.0 label Aug 20, 2024

jughosta mentioned this pull request Nov 5, 2024

[Discover][Embeddable] Allow to customize column widths #196728

Closed

davismcphee linked an issue Nov 5, 2024 that may be closed by this pull request

[Discover][Embeddable] Allow to customize column widths #196728

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

davismcphee commented Aug 12, 2024 •

edited

Loading

davismcphee commented Aug 12, 2024

davismcphee commented Aug 12, 2024

davismcphee commented Aug 13, 2024

davismcphee Aug 13, 2024

davismcphee commented Aug 13, 2024

elasticmachine commented Aug 14, 2024

jughosta left a comment

jughosta Aug 14, 2024

davismcphee Aug 15, 2024

jughosta Aug 14, 2024

davismcphee Aug 15, 2024

davismcphee Aug 15, 2024

jughosta Aug 14, 2024

davismcphee Aug 15, 2024

opauloh left a comment

jughosta left a comment

michaelolo24 left a comment

kibana-ci commented Aug 20, 2024

API count

	colSettings: { columnId: string; width?: number },
	colSettings: { columnId: string; width: number \| undefined },

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

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

Conversation

davismcphee commented Aug 12, 2024 • edited Loading

Summary

Checklist

For maintainers

davismcphee commented Aug 12, 2024

davismcphee commented Aug 12, 2024

davismcphee commented Aug 13, 2024

Choose a reason for hiding this comment

davismcphee commented Aug 13, 2024

elasticmachine commented Aug 14, 2024

jughosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opauloh left a comment

Choose a reason for hiding this comment

jughosta left a comment

Choose a reason for hiding this comment

michaelolo24 left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 20, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

davismcphee commented Aug 12, 2024 •

edited

Loading