[Time to Visualize] Remove Panels from URL by ThomThomson · Pull Request #86939 · elastic/kibana

ThomThomson · 2020-12-24T16:02:56Z

Summary

This PR accomplishes a number of things:

Note: The following changes will only be visible If dashboard.allowByValueEmbeddables is set to true in config/kibana.dev.yml Edit: This option is now set to true by default

Shorter Urls: Dashboard panel state storage in edit mode is no longer handled by the URL. In edit mode, the URL should be significantly shorter.
Stickier State: Since panel state is now stored in Session Storage, you should not lose unsaved state unless you close your tab. Navigating anywhere else in Kibana and back to your dashboard through any means should properly restore your edits.
Multiple Edits: You can now have ongoing unsaved edits to multiple dashboards in the same session and flip between them.
Recently Accessed: The recently accessed section now remembers if you were in view mode or edit mode on the dashboard, and if you were editing it, it restores the panels. New dashboards now also show up in the recently accessed section
Edit Icon The pencil icon in the dashboard listing page is now a shortcut that opens the selected dashboard in edit mode initially.
Callout: A small callout section will appear on the dashboard listing page. It lists dashboards with unsaved edits and provides options to discard the changes, or continue editing.
Discard vs View / Edit: Leaving edit mode no longer prompts the user to discard their changes. You can flip between edit and view mode freely, and there is now an explicit button for discarding your unsaved changes.

To do: Functional Tests are incoming. Required functional tests are linked in #80584

Testing Ideas

This PR has a lot of UX, so it's important we test every single edge case we can think of. Some edge cases I can think of include:

Start editing a dashboard and leave. Any way you can get back to that dashboard should restore your unsaved edits, including: back button, Listing page link, Listing page unsaved callout link, recently accessed link. etc.
Save an existing dashboard, make sure that the unsaved edits to that dashboard have been cleared.
Save an existing dashboard as a new dashboard, and ensure the edits to the original dashboard have been cleared.
Create a new dashboard, ensure its unsaved edits get saved. Save the new dashboard, ensure that the unsaved edits are cleared.
Do a snapshot share, ensure that the state is properly put into the URL. Open this URL in a new tab, it should populate those unsaved changes.
Do a snapshot share with a short URL. That should work the same way as the previous item.
There are probably plenty of other edge cases

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)
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

…ePanelsFromUrl

…its. Changed edit link in listing page to actually edit. Removed unsaved edits when dashboard is not dirty

… is removed on save.

…ePanelsFromUrl

…onstants

elasticmachine · 2021-01-15T02:45:39Z

Pinging @elastic/kibana-presentation (Team:Presentation)

Dosant · 2021-01-15T10:17:11Z

To do: Functional Tests will be added in a follow up PR after #88390. Required functional tests are linked in #80584

You can consider creating a new test config with byValue enabled to merge tests together with such feature pieces. This is an example where we did this for search sessions: #84833

Dosant

Tested a bit, mostly was focused on stuff around state management / URL syncing

Noticed that and assume is by design:

browser history navigation doesn't work anymore more for panel changes

Think those are bugs:

"cancel" in edit mode no longer works for time range changes. I checked 7.10.2 and it worked. Didn't check master, possible it regressed before this pr
Looks like "cancel" does something weird now with state in the URL. removes _g part from the URL and adds multiple not relevant history records
You can get into a bad state if press "back" just after saving a new dashboard. Maybe makes sense to use "replace" there.

src/plugins/dashboard/public/application/lib/dashboard_panel_storage.ts

src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.test.tsx

ThomThomson · 2021-01-15T16:30:24Z

You can consider creating a new test config with byValue enabled to merge tests together with such feature pieces. This is an example where we did this for search sessions: #84833

We actually already have one of these in test/new_visualize_flow. I will most likely include the functional tests in this PR - as it seems like #88390 will be merged first.

I will keep track of the bugs you've discovered - as well as another I found.

Entering edit mode, adding unsaved panels, then pressing back -> forward will remove the added panels without a warning.

src/plugins/dashboard/public/dashboard_strings.ts

poffdeluxe

Looks pretty good to me! However, I did also run into the issue where after saving a new dashboard, hitting the back back button takes me to a blank dashboard

ThomThomson · 2021-01-15T19:11:33Z

Looks pretty good to me! However, I did also run into the issue where after saving a new dashboard, hitting the back back button takes me to a blank dashboard

Will definitely fix all identified issues before merge! Thanks for looking into it

…ePanelsFromUrl

ryankeairns

Approving the SASS and design aspects. As was discussed, there are some nice-to-haves (e.g. discard all) that can be tracked separately. Likewise, we can iterate on the layout once the page layout service becomes available.

Great work here! Losing Dashboards is a longstanding, frustrating experience.

ThomThomson · 2021-01-27T18:15:00Z

@elasticmachine merge upstream

Dosant

Code review and testing:
Found some things that I think worth addressing + some nits.

In this dialog:

if I click "continue editing" it is just closing the dialog. I'd assume it either should navigate to the dashboard or button text should be changed

Edits to dashboard filters/query/time aren't saved when I return to them from the listing. I assume this is expected behavior now, but I thought I'd call it out.
When you edit a dashboard, then go to the listing page, and instead of continue editing it from the draft list navigate to it and then edit or just click edit from a normal list, then you just implicitly jump to the last edit state. I think that could be confusing in some cases and a dialog that explicitly asks if the user wants to restore the latest draft or discard it would be nice to have. Probably related to #88901
Noticed that drafts dialog gets clunky on smaller screen:

Also imo it is still worth adding some error handling on reading/writing sessionStorage.

src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.tsx

Dosant · 2021-01-28T11:18:17Z

src/plugins/dashboard/public/dashboard_constants.ts

-export function createDashboardEditUrl(id: string) {
-  return `${DashboardConstants.VIEW_DASHBOARD_URL}/${id}`;
+export function createDashboardEditUrl(id?: string, editMode?: boolean) {
+  const edit = editMode ? `?${DASHBOARD_STATE_STORAGE_KEY}=(viewMode:edit)` : '';


nit: you can use setStateToKbnUrl function from kibana_utils to set state in proper format

I thought about using this, but it seems to prepend the entire current URL / not be used for routing within apps. I might be using it wrong though.

I thought it should also work like this:

setStateToKbnUrl('_a', {viewMode: 'edit'}, {storeInHashQuery: false}, '/dashboard/dashboard-id')

but I am not 100% sure now. feel free to skip .

p.s. hate this weird fn signature 😅

Yeah that did work, the trick was the storeInHashQuery: false argument. I will leave it in though it does seem to be adding quite a bit of complexity to this function.

I actually had to revert this. Turns out importing the stateToKbnUrl method in that file causes a build failure in x-pack because of a functional test file using this. I ran into this before as well but forgot!

src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.tsx

ryankeairns · 2021-01-28T15:20:25Z

3. Noticed that drafts dialog gets clunky on smaller screen:

@ThomThomson here is a PR with some responsive enhancements. Feel free to merge if it looks good to you.

👉 ThomThomson#4

After

Add responsive styles

ThomThomson · 2021-01-28T17:23:06Z

@ryankeairns, very quick on the draw & looking great. Merged this.

@Dosant, I am working through the suggestions now, but have some initial responses:

Good call, I have changed the wording of the cancel button to just Cancel
Yes, edits to the time range and filters are still part of the URL exclusively. This system could potentially be changed to include all dashboard state. It may save some confusion and is perhaps worth a followup.
When you click on a dashboard which has unsaved edits from the regular listing table (when not using the edit button), you see it in view mode as it was last saved. The unsaved edits are only restored when you enter edit mode. I believe the plan in [Time to Visualize] Indicate unsaved changes #88901 is to have the badge show up in both view & edit mode, which should mitigate this weirdness a lot.
thanks again Ryan!
I've added some really simple error handling to dashboard_panel_storage.ts.

…ePanelsFromUrl

ThomThomson · 2021-02-01T17:14:17Z

@elasticmachine merge upstream

…kibana into feature/removePanelsFromUrl

Dosant

Retested listing page since this is where most of the change since the last review were,
one comment:

Dosant · 2021-02-03T09:04:24Z

src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.tsx

-  useMount(() => {
-    setDashboardIds(dashboardPanelStorage.getDashboardIdsWithUnsavedChanges());
+  useEffect(() => {
+    return () => setMounted(false);


This runs on every state change. If you wanted something like componentWillUnmount then it would have to be implemented a bit different: https://github.com/streamich/react-use/blob/master/src/useUnmount.ts

But as I understand, we need this for request cancelation, then the best would be to handle it as part of the effect with the request

useEffect(() => { let canceled = false; request().then(() => { if (canceled) return; }) return () => { canceled = true; } })

Wow, huh. I meant to pass in an empty dependency array there, which would have made it run only on unmount. I need to be more careful with these things. Thanks for noticing this!

That said, the approach of canceling within the useEffect is cleaner anyway. I have implemented it.

…ePanelsFromUrl

spalger · 2021-02-03T18:20:40Z

jenkins test this

kibanamachine · 2021-02-03T20:06:09Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	200	203	+3

Async chunks

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

id	before	after	diff
`dashboard`	160.9KB	176.4KB	+15.6KB

Page load bundle

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

id	before	after	diff
`dashboard`	295.4KB	299.0KB	+3.5KB
`kibanaReact`	119.4KB	119.4KB	+20.0B
total			+3.5KB

History

💚 Build #102738 succeeded d433d04
💔 Build #102650 failed c714336
💔 Build #102347 failed efd5b3f
💔 Build #101944 failed 0190c26
💚 Build #101787 succeeded 55cda0e

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

Removed panels from dashboard URLs Co-authored-by: Ryan Keairns <contactryank@gmail.com> # Conflicts: # test/functional/apps/dashboard/view_edit.ts

Removed panels from dashboard URLs Co-authored-by: Ryan Keairns <contactryank@gmail.com>

First pass at removing panels from URL. Everything should work

1e2bb42

ThomThomson added release_note:enhancement Feature:Dashboard Dashboard related features Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.0.0 Project:TimeToVisualize v7.12.0 labels Dec 24, 2020

ThomThomson added 4 commits December 24, 2020 14:38

Fix types

8b2ac8b

First version of unsaved panels listing

aa1140e

Merge branch 'master' of github.com:elastic/kibana into feature/remov…

ac24142

…ePanelsFromUrl

type fix

946c7c4

ThomThomson force-pushed the feature/removePanelsFromUrl branch from c12eab3 to 946c7c4 Compare January 12, 2021 19:54

ThomThomson added 3 commits January 12, 2021 18:03

Added confirm to create new when new dashboard already has unsaved ed…

d4b0c11

…its. Changed edit link in listing page to actually edit. Removed unsaved edits when dashboard is not dirty

Fixed typo, ensured that snapshot share links work properly

25da78c

Added unit testing for unsaved listing

ea9162d

ThomThomson force-pushed the feature/removePanelsFromUrl branch from 59a2514 to ea9162d Compare January 14, 2021 17:57

ThomThomson added 3 commits January 14, 2021 16:26

Changed recently accessed functionality. Ensured proper unsaved state…

de19cb7

… is removed on save.

Merge branch 'master' of github.com:elastic/kibana into feature/remov…

1699180

…ePanelsFromUrl

Removed issue caused by dashboard_security importing from dashboard_c…

e5f68d9

…onstants

ThomThomson marked this pull request as ready for review January 15, 2021 02:45

ThomThomson requested review from a team as code owners January 15, 2021 02:45

Dosant reviewed Jan 15, 2021

View reviewed changes

src/plugins/dashboard/public/application/lib/dashboard_panel_storage.ts Show resolved Hide resolved

src/plugins/dashboard/public/application/listing/dashboard_unsaved_listing.test.tsx Show resolved Hide resolved

poffdeluxe reviewed Jan 15, 2021

View reviewed changes

src/plugins/dashboard/public/dashboard_strings.ts Outdated Show resolved Hide resolved

poffdeluxe approved these changes Jan 15, 2021

View reviewed changes

ThomThomson added 2 commits January 27, 2021 10:59

fixed jest

53b2065

Merge branch 'master' of github.com:elastic/kibana into feature/remov…

3f63de0

…ePanelsFromUrl

ryankeairns approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'master' into feature/removePanelsFromUrl

9bdbaa2

Dosant reviewed Jan 28, 2021

View reviewed changes

Dosant mentioned this pull request Jan 28, 2021

[Drilldowns] Enable Drilldowns in edit mode for faster testing #89549

Closed

Add responsive styles

09de507

Merge pull request #4 from ryankeairns/feature/removePanelsFromUrl

55cda0e

Add responsive styles

error handling for dashboard panel storage.

3d1cfeb

ThomThomson mentioned this pull request Jan 28, 2021

[Time To Visualize] Add Functional Tests After Config Option Defaults to True #80584

Closed

33 tasks

ThomThomson added 2 commits January 28, 2021 15:48

Merge branch 'master' of github.com:elastic/kibana into feature/remov…

0190c26

…ePanelsFromUrl

refactor dashboard unsaved listing to show entries when loading

efd5b3f

kibanamachine and others added 3 commits February 1, 2021 12:14

Merge branch 'master' into feature/removePanelsFromUrl

c714336

revert import in dashboard_constants

0bf8e68

Merge branch 'feature/removePanelsFromUrl' of github.com:thomthomson/…

d433d04

…kibana into feature/removePanelsFromUrl

Dosant approved these changes Feb 3, 2021

View reviewed changes

ThomThomson added 2 commits February 3, 2021 11:44

changed unsaved listing cancelation logic

b4355d9

Merge branch 'master' of github.com:elastic/kibana into feature/remov…

c355032

…ePanelsFromUrl

ThomThomson merged commit 9232a5a into elastic:master Feb 3, 2021

ThomThomson mentioned this pull request Feb 3, 2021

[7.x] [Time to Visualize] Remove Panels from URL (#86939) #90247

Merged

ThomThomson added a commit that referenced this pull request Feb 4, 2021

[7.x] [Time to Visualize] Remove Panels from URL (#86939) (#90247)

c78a5a2

Removed panels from dashboard URLs Co-authored-by: Ryan Keairns <contactryank@gmail.com>

ThomThomson mentioned this pull request Mar 2, 2021

[Time to Visualize] popup notification when leaving an unsaved dashboard #65179

Closed

ThomThomson mentioned this pull request Aug 17, 2021

[Dashboard] Use SessionStorage to Persist ViewMode State #108920

Closed

Conversation

ThomThomson commented Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing Ideas

Checklist

For maintainers

Uh oh!

elasticmachine commented Jan 15, 2021

Uh oh!

Dosant commented Jan 15, 2021

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ThomThomson commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

poffdeluxe left a comment

Choose a reason for hiding this comment

Uh oh!

ThomThomson commented Jan 15, 2021

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

ThomThomson commented Jan 27, 2021

Uh oh!

Dosant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dosant Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

ThomThomson Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

Dosant Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

ThomThomson Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

ThomThomson Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryankeairns commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

After

Uh oh!

ThomThomson commented Jan 28, 2021

Uh oh!

ThomThomson commented Feb 1, 2021

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

Dosant Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomThomson Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

spalger commented Feb 3, 2021

Uh oh!

kibanamachine commented Feb 3, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

ThomThomson commented Dec 24, 2020 •

edited

Loading

ThomThomson commented Jan 15, 2021 •

edited

Loading

Dosant left a comment •

edited

Loading

ryankeairns commented Jan 28, 2021 •

edited

Loading

Dosant Feb 3, 2021 •

edited

Loading