elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.72k stars 8.14k forks source link

[discuss] Remove app url changes in the side nav #146318

Open Dosant opened 1 year ago

Dosant commented 1 year ago

Currently Discover, Dashboard and Visualize app (not Lens) use the kbnUrlTracker helper to manipulate the URL to the app's link in the nav bar.

https://github.com/elastic/kibana/blob/74e6282b243960e290865c68a1d0831cd86608f3/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.ts#L46

Example usage in discover:

https://github.com/elastic/kibana/blob/74e6282b243960e290865c68a1d0831cd86608f3/src/plugins/discover/public/utils/initialize_kbn_url_tracking.ts#L44-L93

Original Goals

This functionality had two goals:

Restore app's state when navigating back

The first goal was to preserve app's state when navigating between apps. This is how it worked:

  1. In Discover get into some not default app state.
  2. The app state in Discover is preserved to the current URL
  3. Then navigate to the Dashboard
  4. kbnUrlTracker sees that the user navigates away from the Discover, picks the current URL with all the state, and updates the link to the Discover app putting all the state there
  5. After navigating the Dashboard, user navigates back to Discover using the side nav. As users clicks on the link with the old state in it, Discover opens with the previous state.

This worked through the links because in the past Kibana had hard reloads when navigating between apps (no sure why not session storage). Today it would be more straightforward if this would be implemented simply in-memory. As I understand, since dashboard removed most of the state from the URL, this was re-implemented there using memory or session storage.

Transfer global state between apps (time range and pinned filters)

This was also used to transfer time range and pinned filters changes between apps. When the "global" state changed, all the links in the sidebar where updates with the current time range and pinned filters. This way when user navigated to the app, the state of those was restored from the link.

This is likely redundant now because this state can be kept in-memory in filter manager and time filter service

Why Remove This

Some reasons why I think we should consider removing that functionality. Then apps should decide if they want to drop the feature they got from this or re-implement in a more simple way:

  1. Code is complicated and is messing with the URLs. Can be re-implemented using in-memory and session storage means.
  2. By default everything from the URL is preserved in the link. This leads to unwanted state restorations. This was a thing we hit when trying to enable URL syncing for the table list view https://github.com/elastic/kibana/pull/145517#issuecomment-1320099547 . The workaround could be to manually exclude unwanted url parts like we did for the search session id: https://github.com/elastic/kibana/blob/74e6282b243960e290865c68a1d0831cd86608f3/src/plugins/discover/public/utils/initialize_kbn_url_tracking.ts#L81-L89
  3. The behavior is not consistent across Kibana. Some users even consider this a bug before (https://github.com/elastic/kibana/pull/145517#issuecomment-1320166767). I think ideally long-term we probably should approach this in a holistic way and help to make a consistent behavior across all apps

cc @ThomThomson, @sebelga

elasticmachine commented 1 year ago

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

elasticmachine commented 1 year ago

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

elasticmachine commented 1 year ago

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

sebelga commented 1 year ago

Thanks for opening this issue @Dosant 👍

I personally think that we should avoid as much as possible changing how a user interacts with the browser and what they expect from it. If the main menu link takes me to "/dashboard/home" I expect this link/behaviour to be consistent no mater what' I've been doing before.

If apps see the need to restore state, like you said there are different options, be it memory through a shared service or session storage.

For the UX it can be pretty cool to have a banner on top when navigating back: "Do you want to restore your previous state?" and maybe even some toggles to let the user restore partial states (time range, filters, sorting...)

elasticmachine commented 1 year ago

Pinging @elastic/appex-sharedux (Team:SharedUX)

vadimkibana commented 1 year ago

+1 for not porting this functionality to Serverless, I've always found this "feature" very confusing in the on-prem Kibana; something that we should remove. So, hopefully if we can stop doing this in Serverless that is one step towards also removing it in on-prem.

ThomThomson commented 11 months ago

+1 from the presentation team on removing this functionality. This will help a lot with removing complexity from the app, and will lead to more consistent behaviour.

@Dosant, are we considering moving forward with removing this on prem?