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

Monitoring time picker changes should not be implemented in multiple locations #14190

Closed
thomasneirynck opened this issue Sep 27, 2017 · 2 comments
Labels
chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Sep 27, 2017

We have multiple locations where we monitor changes in the time-picker. This should be consolidated in the timepicker itself

From #12811 (comment):

Responding to changes in the time-picker is currently implemented all over the place.

The reason Discover works is because it relies on courier.

Visualize - after the refactor - should have its own refresher (triggering a reload event on every tick). This should replace the refresher from TSVB (and perhaps Timelion). Alternatively, we could make this optional, so individual visualizations can opt-in/opt-out of the auto-refreshing functionality.

Also consider that this timing loop should run in the timepicker component itself (https://github.com/thomasneirynck/kibana/blob/5c075f2a3f4eb3bebd2ec7a105b5a03ff8abe292/src/ui/public/timepicker/timepicker.js#L210-L210). That way, all clients of the timepicker can listen to an auto-refresh event. Right now, this logic is flipped.

@thomasneirynck thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) chore v6.0.0 labels Sep 27, 2017
@epixa
Copy link
Contributor

epixa commented Sep 27, 2017

Sounds like a job for observables. Timepicker exposes an observable to represent the current time range, and then all the various places that need it create a subscription to it.

@timroes timroes added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Sep 16, 2018
@timroes
Copy link
Contributor

timroes commented Oct 27, 2018

I think this issue is outdated. Visualizations should be triggered correctly from the outside for refreshing. Apps will still need to manage their time picker correctly, optimizations around that, should be tracked for the new timepicker plugin in #18845.

@timroes timroes closed this as completed Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

3 participants