[Discover] Persist hide chart option to local storage #114534

majagrubic · 2021-10-11T17:10:31Z

Summary

Release Notes: Remember most recent "hide chart" setting

Fixes: #112466
This PR stores the save/hide chart option to local storage.

Whenever a saved object is loaded, that setting takes precedence over whatever is in a local storage. When a user manually toggles a chart, that preference is stored in the local storage. It will be used from that point on any time when creating a "new" saved search or switching between index patterns.

Checklist

Delete any items that are not applicable to this PR.

~~- [ ] 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
~~- [ ] 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

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

majagrubic · 2021-10-11T19:03:51Z

@elasticmachine merge upstream

majagrubic · 2021-10-12T09:54:16Z

@elasticmachine merge upstream

elasticmachine · 2021-10-13T11:09:26Z

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

timroes · 2021-10-13T12:06:28Z

With this change the localStorage setting will get priority over the one saved in saved searches, when the chart is hidden. I would vote for having the state saved in the saved object always getting priority over your local state when loading a saved search.

Example: Save a saved search with a shown chart, toggle to a different index pattern, hide the chart now, load the previously saved search: The saved search appears, but the chart is hidden (despite the saved object had a hiddenChart: false in it.

My preferred behavior would be: whenever a saved object is loaded we use that setting. Whenever a user manually toggle a chart, store that new one in the local storage, use the local storage setting anytime when creating a "new" saved search or switching between index patterns, but not when loading a previously saved search.

@kertal @dmitriynj what are your thoughts around that?

kertal · 2021-10-13T14:44:04Z

I would vote for having the state saved in the saved object always getting priority over your local state when loading a saved search.

yes, saved search setting should get priority over local state 👍. local storage persistence should be used for human interaction, and users will be pleased, I'm sure

majagrubic · 2021-10-13T19:31:07Z

lmk if this is more inline with what you described

kertal

Code LGTM, works as expected, tested a-la-carte with Safari, Chrome, Firefox. It's nice useful add-on , a little detail that improves UX in Discover 👍

kertal · 2021-10-14T07:17:38Z

test/functional/apps/discover/_chart_hidden.ts

+    });
+
+    after(async () => {
+      await kibanaServer.uiSettings.unset('timepicker:timeDefaults');


'timepicker:timeDefaults' do we need to reset this? since it has not been set?

Good point, I thought this was unsetting the default time in Discover 🤦‍♀️

kertal · 2021-10-14T07:25:00Z

Additional note: think adding a bit more description to the PR , how it behaves when saved searches are load would be useful

majagrubic · 2021-10-14T13:50:18Z

@elasticmachine merge upstream

kibanamachine · 2021-10-14T15:51:36Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`discover`	328.7KB	328.9KB	+149.0B

History

💛 Build #160787 was flaky 3afb19c
💚 Build #160601 succeeded c1ba94e
💔 Build #160544 failed c443cb5
💚 Build #160348 succeeded 1cffac7
💔 Build #159987 failed 12b250c

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

* Persist hide chart to local storage * [Discover] Persist hide chart option to local storage * Fix state * Fix dependency check * Set chart state to undefined * Update unit test * Do not override saved search preferences * Fix missing import * Add a functional test * Add a functional test * Fix functional test Co-authored-by: Kibana Machine <[email protected]>

) * Persist hide chart to local storage * [Discover] Persist hide chart option to local storage * Fix state * Fix dependency check * Set chart state to undefined * Update unit test * Do not override saved search preferences * Fix missing import * Add a functional test * Add a functional test * Fix functional test Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

Maja Grubic added 2 commits October 11, 2021 18:59

Persist hide chart to local storage

31302c0

[Discover] Persist hide chart option to local storage

07f20ef

majagrubic force-pushed the save-chart-preferences branch from b552682 to 07f20ef Compare October 11, 2021 18:27

kibanamachine and others added 3 commits October 11, 2021 15:04

Merge branch 'master' into save-chart-preferences

bbe9a2c

Fix state

b284d20

Fix dependency check

ca30b2e

kibanamachine and others added 3 commits October 12, 2021 05:54

Merge branch 'master' into save-chart-preferences

8e54395

Set chart state to undefined

12b250c

Update unit test

1cffac7

majagrubic added v8.0.0 v7.16.0 Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. release_note:enhancement labels Oct 13, 2021

majagrubic marked this pull request as ready for review October 13, 2021 11:09

majagrubic requested a review from a team as a code owner October 13, 2021 11:09

kertal added the Feature:Discover Discover Application label Oct 13, 2021

Maja Grubic added 2 commits October 13, 2021 17:34

Do not override saved search preferences

c443cb5

Fix missing import

9f11036

majagrubic marked this pull request as draft October 13, 2021 16:42

Maja Grubic added 2 commits October 13, 2021 18:56

Add a functional test

2713bfc

Add a functional test

c1ba94e

majagrubic marked this pull request as ready for review October 13, 2021 19:30

kertal approved these changes Oct 14, 2021

View reviewed changes

Fix functional test

3afb19c

Merge branch 'master' into save-chart-preferences

51a73cc

majagrubic merged commit 45a0032 into elastic:master Oct 14, 2021

majagrubic deleted the save-chart-preferences branch October 14, 2021 17:22

majagrubic mentioned this pull request Oct 14, 2021

[7.x] [Discover] Persist hide chart option to local storage (#114534) #115064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Persist hide chart option to local storage #114534

[Discover] Persist hide chart option to local storage #114534

majagrubic commented Oct 11, 2021 •

edited by timroes

Loading

majagrubic commented Oct 11, 2021 •

edited

Loading

majagrubic commented Oct 12, 2021

elasticmachine commented Oct 13, 2021

timroes commented Oct 13, 2021

kertal commented Oct 13, 2021

majagrubic commented Oct 13, 2021

kertal left a comment

kertal Oct 14, 2021

majagrubic Oct 14, 2021

kertal commented Oct 14, 2021

majagrubic commented Oct 14, 2021

kibanamachine commented Oct 14, 2021

[Discover] Persist hide chart option to local storage #114534

[Discover] Persist hide chart option to local storage #114534

Conversation

majagrubic commented Oct 11, 2021 • edited by timroes Loading

Summary

Checklist

Risk Matrix

For maintainers

majagrubic commented Oct 11, 2021 • edited Loading

majagrubic commented Oct 12, 2021

elasticmachine commented Oct 13, 2021

timroes commented Oct 13, 2021

kertal commented Oct 13, 2021

majagrubic commented Oct 13, 2021

kertal left a comment

Choose a reason for hiding this comment

kertal Oct 14, 2021

Choose a reason for hiding this comment

majagrubic Oct 14, 2021

Choose a reason for hiding this comment

kertal commented Oct 14, 2021

majagrubic commented Oct 14, 2021

kibanamachine commented Oct 14, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

majagrubic commented Oct 11, 2021 •

edited by timroes

Loading

majagrubic commented Oct 11, 2021 •

edited

Loading