Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable url tracking for dashboard #55818

Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jan 24, 2020

Move URL tracking into helper function controlled by the app plugin. The URL tracker listens to history changes and optionally to global state changes and updates the nav link url of a given app to point to the last visited page within the app.
This includes the following parts:

  • When the app is currently active, the nav link points to the configurable default url of the app.
  • When the app is not active the last visited url is set to the nav link.
  • When a provided observable emits a new value, the state parameter in the url of the nav link is updated
    as long as the app is not active.

This has two parts:

  • The general kbnUrlTracker that handles setting the nav link to the right value in the various scenarios
  • A getQuerySyncStateContainer helper that provides a state container pulling in the latest state from the state in data services. This one is basically a refactoring of a part of the syncState helper and providing just the pull-part of state syncing (without automatic state updates back into the data plugin services)

@flash1293 flash1293 marked this pull request as ready for review January 31, 2020 12:56
@flash1293 flash1293 requested a review from a team January 31, 2020 12:56
@flash1293 flash1293 requested a review from a team as a code owner January 31, 2020 12:56
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jan 31, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed and clicked around a bit. LGTM.
Left some comments.

Concerned about every app having to create each own state container to get updates from data services, which should be improved separately. #56503

filters: filterManager.getGlobalFilters(),
};

export const syncQuery = (queryStart: QueryStart, urlStateStorage: IKbnUrlStateStorage) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for info:
I was also doing something similar in this file in effort of improving this helpers:
#56128 it is not ready yet and waiting for #56160

so afterwards I will merge and unify 👍

src/legacy/core_plugins/kibana/public/dashboard/plugin.ts Outdated Show resolved Hide resolved
src/legacy/core_plugins/kibana/index.js Outdated Show resolved Hide resolved
) {
const { querySyncStateContainer, stop: stopQuerySyncStateContainer } = getQueryStateContainer(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we will have to add this to data plugin as part of the api. Because with current setup, if to apply this for multiple apps: each app will create a state container, which will all the time independently orchestrate data.query changes.
So we should add 1 state container on a data plugin, which apps could reuse and receive the updates.

cc @lizozom, I think you've mentioned something like this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's even worse than that - the syncState helper is also creating its own state container, so even the dashboard plugin alone will have two of them running while the app is mounted. +1 for exposing it once via setup contract and using it everywhere from there.

@flash1293
Copy link
Contributor Author

@Dosant Could you have another look? Changed some things and fixed the linting problem (there were a few things caught by the updated rules I just fixed as well while at it)

@Dosant
Copy link
Contributor

Dosant commented Feb 3, 2020

@Dosant Could you have another look? Changed some things and fixed the linting problem (there were a few things caught by the updated rules I just fixed as well while at it)

@flash1293, reviewed and clicked around again. looks good to me!

@kertal kertal self-requested a review February 4, 2020 06:43
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM 👍 , tested locally with chrome, works.

* * When the app is currently active, the nav link points to the configurable default url of the app.
* * When the app is not active the last visited url is set to the nav link.
* * When a provided observable emits a new value, the state parameter in the url of the nav link is updated
* as long as the app is not active.
Copy link
Member

@kertal kertal Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be a nice addon to the PRs description for giving context what sub url tracking does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @kertal, sorry for leaving this a little cryptic. I will update to document a bit better for future readers

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants