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

Visualize refactor follow up #12930

Closed
9 tasks
thomasneirynck opened this issue Jul 17, 2017 · 4 comments
Closed
9 tasks

Visualize refactor follow up #12930

thomasneirynck opened this issue Jul 17, 2017 · 4 comments
Labels
chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.0.0

Comments

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Jul 17, 2017

The refactor introduced a significant amount of changes.

Keep track here of internal changes/feedback to either tidy up or improve the code-base.

  • AGREED time-picker auto-refresh should not be implemented in multiple locations (cf. Auto-refresh does not work on dashboard and visualize #12811 (comment)).
  • consider trimming the general purpose update_status. The general-purpose serialization adds overhead. Especially for data-refreshes it's an overhead we do not need to incur. We have full-control over the backend and know when the requesthandler is called or not (and thus whether the data is refreshed). This is also buggy, as it flagged data changes where non happened (see Coordinates Map visualization re-creating geohashLayer on map move #12919).
  • UNDER CONSIDERATION consider removing responseHandler. This abstraction does not add immediate benefit. Individual visualizations could just make a function call in the render-method to make this transformation individually. Should not overdesign right of the bat, we can introduce this later.
  • POC new response handlers for vislib #12661. ES data should be converted to something that can be consumed by majority visualizations (table, series) with no coupling to ES (see response handler PR)
  • move shouldQuery logic out of the requestHandler so requestHandlers can be stateless. It would also avoid the bleeding of the vis.reload flag into the requestHandler.
  • OK FOR RENAME rename appState.vis to appState.visState. (what is the reason we are storing this on the appState?)
  • AGREED consolidate all vis-code in single directory: Consolidate all visualize code in single directory #12675
  • some options apply now without having to press play (e.g. tagcloud options, show legend in gauges). Do we want this? It prevents us from using the cancel functionality.
  • editormode should be true when designing a visualization (e.g. with sidebar editor), but false on a dashboard. this doesn't seem to be the case.

cc @ppisljar

@thomasneirynck thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) chore v6.0.0 labels Jul 17, 2017
@ppisljar
Copy link
Member

ppisljar commented Jul 19, 2017

moving shouldQuery out of request handler:
this is gonna be hard, i prefer that requestHandlers have some state and take care of this.
the problem is that the decision to query or not is not a general one, its very specific to each requestHandler.

for example courier requestHandler will only query if aggs have changed ...
tsvb request handler would query much more often (when vis.params change, when uistate changes ...)

i think the same goes thru for responseHandlers as well, so adding some state to them would be my choice

@ppisljar
Copy link
Member

rename appState.vis
this is gonna require changing searchSource, savedObjectLoader and more ... i don't feel comfortable doing that.

the reason we are storing vis state here: we want everything in url (so you can forward/back and copy paste it to others)

@thomasneirynck
Copy link
Contributor Author

thomasneirynck commented Sep 27, 2017

@ppisljar
Copy link
Member

could you link to those tickets here ?

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) v6.0.0
Projects
None yet
Development

No branches or pull requests

2 participants