Refactor and improve Visualize Loader #15157

timroes · 2017-11-27T10:17:56Z

This PR refactors the VisualizeLoader. The aim is to use the loader instead of the <visualize> directive in the future, so that we:

use the API we are currently promoting in webinars also ourselves (which will keep it more up to date), and
can get rid of Angular in more places where, you just want to embed a visualization.

This PR also adds all required methods and functionality, that we could refactor the dashboard to use this API instead of rendering a <visualize> directive itself.

The PR changes roughly the following things:

Return the handler from embedVisualizationWithSavedObject directly instead of after rendering
Add cssClass, dataAttrs and append attributes (required for dashboard)
Removes the editorMode params, since this wasn't working properly anyway (due to us inheriting $rootScope)
Extracting the rendered template into its own file
Use angular.element instead of jQuery
Lot of code refactoring
Adding several tests
Adding documentation and fixing developer documentation

With this PR we would also recommend not to use <visualize> directly anymore. That gives us the option to maybe remove the Angular directive altogether in the long run, or at least being able to "more safely" change it if needed.

Fixes #15146 (the actual root cause - since this ticket is already closed)

This is required to implement #15153

Release Note: Do not use the <visualize> directive anymore to embed a visualization. Use the Visualize Loader instead.

ppisljar

looks really nice, some small updates to the docs would make me very happy :)

ppisljar · 2017-11-28T06:57:35Z

src/ui/public/visualize/loader.js

+     * @return {Promise.<EmbeddedVisualizeHandler>} A promise that resolves to the
+     *    handler for this visualization as soon as the saved object could be found.
+     */
+    embedVisualizationWithId: async (element, id, params) => {


id vs savedVisualizationId .... well i guess as we have the docs now its quite clear, but in general our style guide directs to using variable names that are descriptive

ppisljar · 2017-11-28T07:04:40Z

docs/development/visualize/development-embedding-visualizations.asciidoc

-which is explained later in this documentation.
+- `getVisualizationList()`: which returns promise which gets resolved with a list of saved visualizations
+- `embedVisualizationWithId(container, savedId, params)`: which embeds visualization by id
+- `embedVisualizationWithSavedObject(container, savedObject, params)`: which embeds visualization from saved object


i think the parts describing the params of directive should be moved here as well (not just deleted) ...
like time-range, showSpyPanel, uiState and appState should probably have some explanation, as well as newly introduced class and data attributes that can be passed in.

ah sorry, i missed the part where you reference the source code and that all parameters are actually described there. ignore my above comment

ppisljar · 2017-11-28T07:42:03Z

will this resolve the IE problem as well (#15150) ?

ppisljar

LGTM, waiting for green CI

can you test if IE problem is resolved (i think it should be, as the container.html('') was actually causing that

timroes · 2017-11-28T14:01:14Z

I added more documentation on the appState and renamed the parameter. I can tested and can confirm, that this PR fixes #15146 .

stacey-gammon · 2017-11-29T15:06:09Z

Why is this labeled as blocked @timroes? Should I still review, or wait till it's unblocked?

timroes · 2017-11-29T15:08:38Z

@stacey-gammon I am currently needing to fix the way we get access to angular with another PR. I will ping you once I need another review. Thanks!

This reverts commit 160e47d.

timroes · 2017-11-30T21:22:35Z

@stacey-gammon @thomasneirynck This PR is now ready for a review. It now uses the chrome.dangerouslyGetActiveInejctor method we introduced in #15267

I also fixed a small bug in the test_utils, that slipped that PR that time with this PR.

thomasneirynck · 2017-12-01T16:10:13Z

src/ui/public/visualize/loader.js

+   * This is especially useful if you used `append: true` in the parameters where
+   * the visualization will be appended to the specified container.
+   */
+  get element() {


I'm not a fan of native getters in Javascript, at least not now. They really hurt navigability in most IDEs, and do not make transparent if you are getting a direct reference to an object or a copy.

e.g. property access in Javascript is pretty straightforward. But with getters we introduce pseudo-properties, where the semantics are just unnecessarily convoluted (especially because IDEs aren't good at picking up on it)

if (myInstance.myProperty === myInstance.myProperty){ //?!?!?! maybe, maybe not. }

When using a method call , e.g. myInstance.getMyProperty(), users don't make as many assumptions. The other advantage is that most Javascript IDEs are getting pretty decent at navigating across types when using methods as opposed to property-access.

My main reason to use a property was, to give read only access to the element. So I am fine by replacing it with a getElement() method.

thomasneirynck

This is a pretty great change, 🙇‍♂️ !

Left some minor comments for your consideration.

thomasneirynck · 2017-12-01T16:20:06Z

src/ui/public/visualize/loader.js

+    this._element = element;
+    this._scope = scope;
+    this._renderComplete = new Promise(resolve => {
+      this._element.on('renderComplete', resolve);


This relies on the assumption visualizations only fire renderComplete once. Which they don't. (e.g. maps are interactive, and modify the UI-state, rerending).

I think we can make this assumption in this context (people need to embed and are just interested in the first "success" status), but then we should immediately remove the listener after resolving.

Hmm good point. I am not sure if people will just need to listen for the first render. So I would actually check with some people actually already using this API, to see what their use cases would be. I would either use the promise, rename the method and only listen once, or alternatively implement a proper callback mechanism to register and deregister for renderComplete events. But I would rather have people to use methods of this API, than needing to listen for the hardcoded string themselves, which will make it harder for us later to change implementations.

Maybe we could also for convenience do both, offer the whenFirstRenderComplete() promise and the event mechanism.

@kobelb - care to pop in here and just make sure the renderComplete stuff should work? I confirmed the tests for print layout pass, and that reports with TSVB and timelion are generated as expected. I think that would trigger an issue if render complete wasn't being counted accurately, but I'm not 100% sure.

thomasneirynck · 2017-12-01T16:21:22Z

src/ui/public/visualize/loader.js

+   * @returns {Promise} Promise, that resolves as soon as the visualization is done rendering.
+   */
+  onRenderComplete() {
+    return this._renderComplete;


rename to whenRenderComplete, because it is only a single promise, set in the constructor.

thomasneirynck · 2017-12-01T16:29:44Z

src/ui/public/visualize/loader.js

+     *    You can alternatively pass a jQuery element instead.
+     * @param {Object} savedObj The savedObject as it could be retrieved by the
+     *    `savedVisualizations` service.
+     * @param {Object} params A list of paramters that will influence rendering.


Since we are rusing the shape of this options object, I'd consider using the typedef functionality to describe this shape instead. http://usejsdoc.org/tags-typedef.html

Then you can just reference it in your params, like

@typedef EmbedVisualizationParams @property {AppState} appState .... //and then reuse it in your method docs @param {EmbedVisualizationParams} params

Will change this, thanks.

stacey-gammon

Just to confirm, the changes to how the embeddable_factory renders the visualize template is going to follow suit in a part 2?

I checked the sharing stuff, and all seems to work as expected.

I wonder how this will eventually fit in with the dashboard embeddable stuff. I actually just recently renamed a class called EmbeddableVisualizeHandler to EmbeddableVisualizeFactory, so no longer would be a naming conflict, though in my final vision (for now), I have a class called EmbeddableHandler.

It all feels very similar, though I don't know enough about the visualize stuff right now to know how or whether it would converge.

stacey-gammon · 2017-12-01T18:48:15Z

src/ui/public/visualize/loader.js

+ * A handler to the embedded visualization. It offers several methods to interact
+ * with the visualization.
+ */
+class EmbeddedVisualizeHandler {


nit, take it or leave it, but I think this class would be more discoverable in its own file.

Yeah, I already thought about that too. And since you suggest it too, I will do so :)

stacey-gammon · 2017-12-01T19:04:36Z

src/ui/public/visualize/loader.js

+    this._element = element;
+    this._scope = scope;
+    this._renderComplete = new Promise(resolve => {
+      this._element.on('renderComplete', resolve);


@kobelb - care to pop in here and just make sure the renderComplete stuff should work? I confirmed the tests for print layout pass, and that reports with TSVB and timelion are generated as expected. I think that would trigger an issue if render complete wasn't being counted accurately, but I'm not 100% sure.

timroes · 2017-12-01T20:29:19Z

I created an own folder for the loader to split up files, added an listener interface for renderComplete besides the promise (that I renamed) and improved the documentation. So you've got some veto time, if something's not okay for you :-)

* Simplify promise setup logic * Import template from own file * Use angular.element instead of jquery * Add documentation for loader methods * Add params.append * Remove params.editorMode * Clarify when returned promise resolves * Add element to handler * Allow setting CSS class via loader * Use render-counter on visualize * Use Angular run method to get access to Private service * Allow adding data-attributes to the vis element * Refactor loader to return an EmbeddedVisualizeHandler instance * Use this.destroy for previous API * Remove fallback then method, due to bugs * Reject promise from withId when id not found * Add tests * Change developer documentation * Revert "Use Angular run method to get access to Private service" This reverts commit 160e47d. * Rename parameter for more clarity * Add more documentation about appState * Fix broken test utils * Use chrome to get access to Angular * Move loader to its own folder * Use a method instead of getter for element * Add listeners for renderComplete events * Use typedef to document params * Fix documentation

This reverts commit bead358. This backport was botched, so a bunch of inline diffs were committed.

epixa · 2017-12-02T19:15:33Z

The 6.x backported was just reverted due to a bad backport, so please submit a new backport with the conflicts resolved.

* Simplify promise setup logic * Import template from own file * Use angular.element instead of jquery * Add documentation for loader methods * Add params.append * Remove params.editorMode * Clarify when returned promise resolves * Add element to handler * Allow setting CSS class via loader * Use render-counter on visualize * Use Angular run method to get access to Private service * Allow adding data-attributes to the vis element * Refactor loader to return an EmbeddedVisualizeHandler instance * Use this.destroy for previous API * Remove fallback then method, due to bugs * Reject promise from withId when id not found * Add tests * Change developer documentation * Revert "Use Angular run method to get access to Private service" This reverts commit 160e47d. * Rename parameter for more clarity * Add more documentation about appState * Fix broken test utils * Use chrome to get access to Angular * Move loader to its own folder * Use a method instead of getter for element * Add listeners for renderComplete events * Use typedef to document params * Fix documentation

timroes added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) chore release_note:enhancement v6.2.0 v7.0.0 labels Nov 27, 2017

timroes requested review from thomasneirynck, ppisljar and stacey-gammon November 27, 2017 10:17

timroes changed the title ~~Refactor visualize loader~~ Refactor and improve Visualize Loader Nov 27, 2017

ppisljar requested changes Nov 28, 2017

View reviewed changes

ppisljar approved these changes Nov 28, 2017

View reviewed changes

ppisljar mentioned this pull request Nov 28, 2017

fixes the visualizeLoader error in IE #15150

Merged

timroes added the blocked label Nov 29, 2017

timroes added the Feature:Vis Loader Visualize loader APIs label Nov 29, 2017

Tim Roes added 12 commits November 30, 2017 21:46

Simplify promise setup logic

41696ad

Import template from own file

20b7beb

Use angular.element instead of jquery

8781d30

Add documentation for loader methods

0ea987f

Add params.append

356b595

Remove params.editorMode

b952e17

Clarify when returned promise resolves

80a3289

Add element to handler

5973c2f

Allow setting CSS class via loader

e0f5035

Use render-counter on visualize

1c87ef1

Use Angular run method to get access to Private service

8b891ad

Allow adding data-attributes to the vis element

a017fd9

Tim Roes added 6 commits November 30, 2017 21:46

Change developer documentation

dd65312

Revert "Use Angular run method to get access to Private service"

49ca6d4

This reverts commit 160e47d.

Rename parameter for more clarity

c6f21b6

Add more documentation about appState

c667336

Fix broken test utils

ecba46c

Use chrome to get access to Angular

cafdda8

timroes force-pushed the refactor-visualize-loader branch from d19cba5 to cafdda8 Compare November 30, 2017 21:04

timroes removed the blocked label Nov 30, 2017

thomasneirynck reviewed Dec 1, 2017

View reviewed changes

stacey-gammon reviewed Dec 1, 2017

View reviewed changes

stacey-gammon approved these changes Dec 1, 2017

View reviewed changes

Tim Roes added 4 commits December 1, 2017 20:39

Move loader to its own folder

0f8ed06

Use a method instead of getter for element

30d2c75

Add listeners for renderComplete events

71e3eb7

Use typedef to document params

01a673d

Fix documentation

fd0758a

timroes merged commit e5d2ff8 into elastic:master Dec 1, 2017

timroes deleted the refactor-visualize-loader branch December 1, 2017 23:44

timroes mentioned this pull request Dec 1, 2017

[6.x] Refactor and improve Visualize Loader (#15157) #15359

Merged

epixa added a commit that referenced this pull request Dec 2, 2017

Revert "Refactor and improve Visualize Loader (#15157) (#15359)"

fa3c2d5

This reverts commit bead358. This backport was botched, so a bunch of inline diffs were committed.

timroes mentioned this pull request Dec 2, 2017

[6.x] Refactor and improve Visualize Loader (#15157) #15362

Merged

timroes mentioned this pull request Jan 8, 2018

Fix broken loader in 6.1 #15885

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and improve Visualize Loader #15157

Refactor and improve Visualize Loader #15157

timroes commented Nov 27, 2017 •

edited

ppisljar left a comment

ppisljar Nov 28, 2017

ppisljar Nov 28, 2017

ppisljar Nov 28, 2017

ppisljar commented Nov 28, 2017

ppisljar left a comment •

edited

timroes commented Nov 28, 2017 •

edited

stacey-gammon commented Nov 29, 2017

timroes commented Nov 29, 2017

timroes commented Nov 30, 2017

thomasneirynck Dec 1, 2017 •

edited

timroes Dec 1, 2017

thomasneirynck left a comment

thomasneirynck Dec 1, 2017

timroes Dec 1, 2017

timroes Dec 1, 2017

stacey-gammon Dec 1, 2017

thomasneirynck Dec 1, 2017

thomasneirynck Dec 1, 2017

timroes Dec 1, 2017

stacey-gammon left a comment

stacey-gammon Dec 1, 2017

timroes Dec 1, 2017

stacey-gammon Dec 1, 2017

timroes commented Dec 1, 2017

epixa commented Dec 2, 2017

Refactor and improve Visualize Loader #15157

Refactor and improve Visualize Loader #15157

Conversation

timroes commented Nov 27, 2017 • edited

ppisljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar commented Nov 28, 2017

ppisljar left a comment • edited

Choose a reason for hiding this comment

timroes commented Nov 28, 2017 • edited

stacey-gammon commented Nov 29, 2017

timroes commented Nov 29, 2017

timroes commented Nov 30, 2017

thomasneirynck Dec 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes commented Dec 1, 2017

epixa commented Dec 2, 2017

timroes commented Nov 27, 2017 •

edited

ppisljar left a comment •

edited

timroes commented Nov 28, 2017 •

edited

thomasneirynck Dec 1, 2017 •

edited