[UnifiedDataTable] Add density configuration #188495
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukasolson
added
release_note:feature
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
10 tasks
davismcphee
reviewed
Jul 24, 2024
There was a problem hiding this comment.
The code changes look good and it's working well! Just a couple pieces of feedback:
We discussed this a bit already, but I think we should normalize the relevant density style options to a single density value when storing them. That way we can modify grid cell styles in the future without them being overwritten by the value in local storage.
We store existing grid options in the saved search and app state so they're restored when opening or adding to a dashboard, and we discussed doing the same for this one in the issue. But I saw @MichaelMarcialis mention in #173155 (comment) that it might make sense not to persist some options. WDYT @MichaelMarcialis, should we persist this one to the saved search or just the browser? Maybe one reason not to is that it might look odd having multiple searches with different densities on a dashboard.
There was a problem hiding this comment.
This makes me realize we have not been keeping the Unified Data Table docs updated..
IMO If we don't persist it we should add some information about this, because we currently persist all configured options.
that sounds like a good reason not to do so |
Yeah that might be a good idea. Maybe we could consider separating options under titles like "Search options" and "Display options" to help draw a distinction? Otherwise I defer to our designer friends to help guide us here 🙂 |
…bana into unified_data_table_density
|
Hey @lukasolson ! Thanks for doing this work, currently it doesn't look like the functionality for actually implementing the data grid density exists in the timeline. Wondering when the planned GA for this functionality might be so we can keep consistency for the time being. Don't want to block this work from going in, but want to be able to plan the work on our side: 
|
davismcphee
reviewed
Aug 10, 2024
There was a problem hiding this comment.
Apologies for the delay getting back to this, the latest changes are looking good overall! I just encountered a few issues while testing to look into. I also still think some functional tests similar to the ones in test/functional/apps/discover/group2_data_grid3/_data_grid_sample_size.ts would be helpful to prevent regressions.
I feel like this wasn't happening last review, but for some reason the values are now cut off for normal and expanded densities. I thought maybe it was an EUI issue but it seems to work ok in their docs:
src/plugins/discover/public/embeddable/components/search_embeddable_grid_component.tsx
Outdated
Show resolved
Hide resolved
packages/kbn-unified-data-table/src/hooks/use_data_grid_density.ts
Outdated
Show resolved
Hide resolved
|
@michaelolo24 Our plan was to merge this as soon as the PR is ready, so likely 8.16 for stateful. By consistency do you mean you want to make sure Timeline supports it too for the same release? |
👍🏾 Yep, that was my only concern (not huge as I wouldn't consider this a critical feature on our side, but want to keep things from diverging as much as possible, anyways it was trivial to get set up on our side: lukasolson#21 ) I'm not going to merge the changes into your PR directly @lukasolson or commit to your branch becasue the new setting might break some tests on our side that I want to work through and not delay your work... but just made the draft PR for reference. 👍🏾 nice work! |
michaelolo24
approved these changes
Aug 12, 2024
Done in c37f61e. |
davismcphee
approved these changes
Aug 15, 2024
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @lukasolson |
kibanamachine
added
v8.16.0
backport:skip
opauloh
added a commit
that referenced
this pull request
Aug 23, 2024
## Summary Due to a [recent change](#188495) on the UnifiedDataTable component, the density was set to default to `compact`. This PR overrides this property to `expanded` in order to keep consistency with the Design specs. ## Screenshot ### Before PR changes:  ### After PR changes:  ### Design spec 
7 tasks
davismcphee
added a commit
that referenced
this pull request
Dec 2, 2024
## Summary When the data grid density setting was added, an issue was found with resetting the density state: #188495 (comment). It was caused by an EUI bug: elastic/eui#7962. The EUI bug has been fixed and this is no longer an issue, so we can remove the workaround we were using by passing the data grid density in the `key` prop to the data grid to force a re-render. Besides removing a small piece of tech debt, this change improves performance when switching densities, and no longer closes the popover when changing the density (like other settings). ### 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 - [ ] [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 - [ ] 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 was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 2, 2024
…#202390) ## Summary When the data grid density setting was added, an issue was found with resetting the density state: elastic#188495 (comment). It was caused by an EUI bug: elastic/eui#7962. The EUI bug has been fixed and this is no longer an issue, so we can remove the workaround we were using by passing the data grid density in the `key` prop to the data grid to force a re-render. Besides removing a small piece of tech debt, this change improves performance when switching densities, and no longer closes the popover when changing the density (like other settings). ### 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 - [ ] [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 - [ ] 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 was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 281269f)
kibanamachine
added a commit
that referenced
this pull request
Dec 2, 2024
…y` (#202390) (#202563) # Backport This will backport the following commits from `main` to `8.x`: - [[Unified Data Table] Stop passing data grid density as `key` (#202390)](#202390) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Davis McPhee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-02T18:22:53Z","message":"[Unified Data Table] Stop passing data grid density as `key` (#202390)\n\n## Summary\r\n\r\nWhen the data grid density setting was added, an issue was found with\r\nresetting the density state:\r\nhttps://github.com//pull/188495#discussion_r1712510252. It\r\nwas caused by an EUI bug: https://github.com/elastic/eui/issues/7962.\r\nThe EUI bug has been fixed and this is no longer an issue, so we can\r\nremove the workaround we were using by passing the data grid density in\r\nthe `key` prop to the data grid to force a re-render. Besides removing a\r\nsmall piece of tech debt, this change improves performance when\r\nswitching densities, and no longer closes the popover when changing the\r\ndensity (like other settings).\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This was checked for breaking HTTP API changes, and any breaking\r\nchanges have been approved by the breaking-change committee. The\r\n`release_note:breaking` label should be applied in these situations.\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"281269f6a050458532b17235df1536984ee0d394","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:DataDiscovery","backport:prev-minor","Feature:UnifiedDataTable"],"title":"[Unified Data Table] Stop passing data grid density as `key`","number":202390,"url":"https://github.com/elastic/kibana/pull/202390","mergeCommit":{"message":"[Unified Data Table] Stop passing data grid density as `key` (#202390)\n\n## Summary\r\n\r\nWhen the data grid density setting was added, an issue was found with\r\nresetting the density state:\r\nhttps://github.com//pull/188495#discussion_r1712510252. It\r\nwas caused by an EUI bug: https://github.com/elastic/eui/issues/7962.\r\nThe EUI bug has been fixed and this is no longer an issue, so we can\r\nremove the workaround we were using by passing the data grid density in\r\nthe `key` prop to the data grid to force a re-render. Besides removing a\r\nsmall piece of tech debt, this change improves performance when\r\nswitching densities, and no longer closes the popover when changing the\r\ndensity (like other settings).\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This was checked for breaking HTTP API changes, and any breaking\r\nchanges have been approved by the breaking-change committee. The\r\n`release_note:breaking` label should be applied in these situations.\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"281269f6a050458532b17235df1536984ee0d394"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202390","number":202390,"mergeCommit":{"message":"[Unified Data Table] Stop passing data grid density as `key` (#202390)\n\n## Summary\r\n\r\nWhen the data grid density setting was added, an issue was found with\r\nresetting the density state:\r\nhttps://github.com//pull/188495#discussion_r1712510252. It\r\nwas caused by an EUI bug: https://github.com/elastic/eui/issues/7962.\r\nThe EUI bug has been fixed and this is no longer an issue, so we can\r\nremove the workaround we were using by passing the data grid density in\r\nthe `key` prop to the data grid to force a re-render. Besides removing a\r\nsmall piece of tech debt, this change improves performance when\r\nswitching densities, and no longer closes the popover when changing the\r\ndensity (like other settings).\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This was checked for breaking HTTP API changes, and any breaking\r\nchanges have been approved by the breaking-change committee. The\r\n`release_note:breaking` label should be applied in these situations.\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [ ] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"281269f6a050458532b17235df1536984ee0d394"}}]}] BACKPORT--> Co-authored-by: Davis McPhee <[email protected]>
hop-dev
pushed a commit
to hop-dev/kibana
that referenced
this pull request
Dec 5, 2024
…#202390) ## Summary When the data grid density setting was added, an issue was found with resetting the density state: elastic#188495 (comment). It was caused by an EUI bug: elastic/eui#7962. The EUI bug has been fixed and this is no longer an issue, so we can remove the workaround we were using by passing the data grid density in the `key` prop to the data grid to force a re-render. Besides removing a small piece of tech debt, this change improves performance when switching densities, and no longer closes the popover when changing the density (like other settings). ### 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 - [ ] [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 - [ ] 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 was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Dec 9, 2024
…#202390) ## Summary When the data grid density setting was added, an issue was found with resetting the density state: elastic#188495 (comment). It was caused by an EUI bug: elastic/eui#7962. The EUI bug has been fixed and this is no longer an issue, so we can remove the workaround we were using by passing the data grid density in the `key` prop to the data grid to force a re-render. Besides removing a small piece of tech debt, this change improves performance when switching densities, and no longer closes the popover when changing the density (like other settings). ### 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 - [ ] [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 - [ ] 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 was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Dec 12, 2024
…#202390) ## Summary When the data grid density setting was added, an issue was found with resetting the density state: elastic#188495 (comment). It was caused by an EUI bug: elastic/eui#7962. The EUI bug has been fixed and this is no longer an issue, so we can remove the workaround we were using by passing the data grid density in the `key` prop to the data grid to force a re-render. Besides removing a small piece of tech debt, this change improves performance when switching densities, and no longer closes the popover when changing the density (like other settings). ### 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 - [ ] [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 - [ ] 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 was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #186007.
Adds a density configuration for the
UnifiedDataTable. By default, this configuration will not be shown unless anonUpdateDataGridDensityhandler is passed to theUnifiedDataTable. It defaults tocompact. It persists tolocalStoragewhen updated.Screen.Recording.2024-07-16.at.5.29.48.PM.mov
Checklist
Delete any items that are not applicable to this PR.
For maintainers