Skip to content

Commit

Permalink
[8.x] [Unified Data Table] Stop passing data grid density as `ke…
Browse files Browse the repository at this point in the history
…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]>
  • Loading branch information
kibanamachine and davismcphee authored Dec 2, 2024
1 parent 3dc759f commit 7e3b70e
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 6 deletions.
2 changes: 0 additions & 2 deletions packages/kbn-unified-data-table/src/components/data_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1155,8 +1155,6 @@ export const UnifiedDataTable = ({
/>
) : (
<EuiDataGridMemoized
// Using this as the `key` is a workaround for https://github.com/elastic/eui/issues/7962. This forces a re-render if the density is changed.
key={dataGridDensity}
id={dataGridId}
aria-describedby={randomId}
aria-labelledby={ariaLabelledBy}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,14 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await dataGrid.changeDensityValue('Normal');

// toggle the popover
// Right now changing the density closes the popover (see packages/kbn-unified-data-table/src/components/data_table.tsx:1144)
// When that workaround is removed we will need to uncomment this next line
// await dataGrid.clickGridSettings();
await dataGrid.clickGridSettings();
await dataGrid.clickGridSettings();
expect(await dataGrid.getCurrentDensityValue()).to.be('Normal');
});

it('should persist the density selection after reloading the page', async () => {
await dataGrid.clickGridSettings();
await dataGrid.changeDensityValue('Expanded');
await dataGrid.clickGridSettings();
expect(await dataGrid.getCurrentDensityValue()).to.be('Expanded');

await browser.refresh();
Expand Down

0 comments on commit 7e3b70e

Please sign in to comment.