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 #11001

Merged
merged 18 commits into from
May 15, 2017
Merged

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Apr 3, 2017

this PR was merged into feature/visualize branch and replaced by this PR: #11786

searching is moved out of visualize editor app (kibana/public/visualize/editor) into visualize component (ui/public/visualize) which was already handling search response.

this simplifies the code (some weird event handling was happening there before to communicate changes between visualize app editor, component and side editor.

it also makes using component easier (embedding it in other apps). before you would need to do all the same things visualize editor app does (handling the search). now all you need to do is provide savedVis and states:

<visualize saved-vis="savedVisObject" app-state="appState" ui-state="uiState" />

i am undecided if you (the embedding app) should be responsible for providing access to filter bar, time picker and query bar or should component be aware of those things ? the following option makes embedding a lot easier. we could still provide the possibility for the embedding app to override some of that behavior.

oh, and this also solves the double rendering (at least in some scenarios). before if you would in any point series chart change a chart type (from line to bar for example) you would see that it double renders (you would first see the line chart and then bar chart ... keeps happening on any other option change)

this is part 1 of visualize changes i want to do ... but i want to get some early feedback as other parts will be based on this.

i tried to capture the changes in this diagram:
screenshot-www draw io 2017-04-03 14-02-35

some more diagrams of the new approach:

simple components with very specific functions:
components

one way data flow:
flow

@@ -183,15 +183,19 @@ function VisEditor($scope, $route, timefilter, AppState, $window, kbnUrl, courie

$scope.timefilter = timefilter;
$scope.opts = _.pick($scope, 'doSave', 'savedVis', 'shareData', 'timefilter', 'isAddToDashMode');
vis.api = {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is here so that component doesn't need to know about this top level components directly, however it depends on them ... i am not sure if i should just move this inside component ?

Copy link
Contributor

@spalger spalger Apr 3, 2017

Choose a reason for hiding this comment

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

Mutating the vis like this isn't a good idea. If <visualize> is going to use the timeFilter and queryFilter it should explicitly/outwardly depend on them.

@ppisljar ppisljar force-pushed the visualize/refactor branch from ec9b392 to 66b847d Compare April 3, 2017 11:41
@tbragin tbragin added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Apr 4, 2017
@ppisljar ppisljar force-pushed the visualize/refactor branch from 4b8c588 to 2a1f400 Compare April 11, 2017 10:54
@ppisljar
Copy link
Member Author

using visualizations inside of other apps

<visualize> component:

  • used to show saved visualizations
  • receives savedVis and appState as input
  • knows a lot about kibana, like time picker, filter panel and query bar.
  • it monitor those and is responsible for calling approp. request and response handlers

usage:

<visualize saved-vis="savedVisObject" app-state="appStateObject" />

<visualization> component

  • used to show visualizations on-the-fly
  • receives data, config and uiState
  • only knows how to render visualization, nothing about kibana

usage:

<visualization data="data" config="config" ui-state="uiState" />

creating new visualizations

new basic visualization type (like metric, tagcloud, table)

  • create and register new visType
  • render function which receives .... and renders the chart
  • editorConfig and editorTemplate

advanced visualization type (like timelion, thor)

  • create and register new Editor, RequestHandler and ResponseHandler
  • all of the basic vis type things

new vislib visualization

  • extend vislib to do something new
  • create and register new VisType extending VislibVisType
  • all you need to provide is editorConfig and editorTemplate

@ppisljar ppisljar force-pushed the visualize/refactor branch from 46b720c to 32c45eb Compare April 14, 2017 12:49
@ppisljar
Copy link
Member Author

at this point this completely breaks every existing visualization (mostly due to renaming of properties and moving files around)

@thomasneirynck
Copy link
Contributor

thomasneirynck commented May 8, 2017

[EDIT] nevermind, this refers to the above error: #11001 (comment)


I'm having trouble running the code, getting the following error when creating a new visualization.

Uncaught TypeError: visType is not a function
    at new VisConfig (:5601/cqg/bundles/kibana.bundle.js?v=8467:113326)
    at Vis.render (:5601/cqg/bundles/kibana.bundle.js?v=8467:111810)
    at VislibVisType.render (:5601/cqg/bundles/kibana.bundle.js?v=8467:159298)
    at :5601/cqg/bundles/kibana.bundle.js?v=8467:117666
    at complete (:5601/cqg/bundles/commons.bundle.js?v=8467:7917)
    at delayed (:5601/cqg/bundles/commons.bundle.js?v=8467:7927)

@ppisljar ppisljar force-pushed the visualize/refactor branch from 32c45eb to 9d26152 Compare May 9, 2017 09:08
@ppisljar ppisljar force-pushed the visualize/refactor branch from 6c53f37 to 27f4dfa Compare May 10, 2017 10:17
@ppisljar ppisljar force-pushed the visualize/refactor branch from 8821b81 to 0fcb0af Compare May 11, 2017 14:54
@ppisljar ppisljar force-pushed the visualize/refactor branch 2 times, most recently from 972d2d4 to 5b221b0 Compare May 12, 2017 13:08
@ppisljar ppisljar force-pushed the visualize/refactor branch from 5b221b0 to 7253333 Compare May 15, 2017 08:58
@ppisljar ppisljar changed the base branch from master to feature/visualize May 15, 2017 08:59
@ppisljar ppisljar merged commit 9fe9197 into elastic:feature/visualize May 15, 2017
ppisljar added a commit that referenced this pull request May 15, 2017
@ppisljar ppisljar changed the title [WIP] Visualize refactor Visualize refactor Jun 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.0.0-alpha2 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants