Create the concept of embeddableHandlers #12146

stacey-gammon · 2017-06-02T17:34:14Z

Register embeddable handlers to push rendering logic outside of dashboard.

As dashboard attempts to render each panel, it will use panel type to grab the corresponding embeddable handler. The actual rendering logic is inside of the embeddableHandler which gets passed a dom node, along with some actions.

stacey-gammon · 2017-06-06T14:49:53Z

jenkins, test this

stacey-gammon · 2017-06-12T12:55:36Z

@thomasneirynck @kjbekkelund, do you guys think you will have a chance to review this over the next week or so? It's not a big rush but would help move things along if I could get it checked in.

kimjoar

This approach looks sane to me, and it all seems to work as expected.

Should probably be reviewed by someone with more Angular skills than me, though, to double-check the Angular-isms.

Should there be tests for each of the embeddable handlers too?

I had a tiny nit, but it all LGTM.

kimjoar · 2017-06-15T12:04:46Z

src/core_plugins/kibana/public/dashboard/panel/panel.js

-        }
+        const type = $scope.panel.type;
+        const id = $scope.panel.id;
+        const service = _.find(services, { type: type });


nitpick (and I know it's just copy/pasted from the old code): If we're still on the "use less lodash", this can be services.find(service => service.type === type).

stacey-gammon · 2017-06-15T16:41:22Z

@spalger - do you have interest in giving this a look over?

thomasneirynck

I genuinely like the idea. I agree that this lower-level of abstraction is a better fit for all the possible dashboard panels, given what we have in Kibana now. The visualize refactor can for now work in isolation just for the Visualize components and doesn't need to accomodate saved searches.

As for this PR. I looked at it only briefly so far, but nothing major jumped out.

I would consider creating an abstract class of an EmbeddableHandler that defines the shape of the type. All children (Visualize/Search/...) then subclass from it. The duck-typing of the EmbedablleHandler-type now works, but having the inheritance would help navigability and would be a form of documentation too.

I'd also add, beyond the 'render', `getEditPath', ... two new methods:

a destroy method, that gets called for all when people navigate away from the dashboard.
a resize(), that instructs an embeddalbe to fit to the dimension of the new surrounding container.

This would match what we're working towards for the shape of the visualization-object in "Visualize". I think these are generic enough to apply to any UI-component.

Do we have to pass in the panel object in getTitlefor/getEditPath. Why not just the 'panel-id'?

For each EmbeddableHandler, consider making name and title constants in the module-scope. These are immutable properties, so they don't need to be instance-properties assigned in the cosntructor.

Embedding a Coordinate Map on the dashboard causes with a TypeError. Cannot read get of undefined on this line https://github.com/thomasneirynck/kibana/blob/262bcb411acc6269dbd0fba519bbee3f96b0e363/src/ui/public/vis_maps/maps_renderbot.js#L58-L58.

I applied these changes to the feature/visualize branch, but there were quite a few merge conflicts, which I didn't have the time to fully resolve. Before we merge either, we should ensure that both PRs will work (this can be merged first no problem of course, but we should check if the feature/visualize branch is compatible)

stacey-gammon · 2017-06-19T19:15:01Z

I would consider creating an abstract class of an EmbeddableHandler that defines the shape of the type. All children (Visualize/Search/...) then subclass from it. The duck-typing of the EmbedablleHandler-type now works, but having the inheritance would help navigability and would be a form of documentation too.

@thomasneirynck - I don't think we have any precedent for this in our code base currently. I'm personally a fan of it, and I used interfaces with typescript at a prior company, but when I've done that type of thing here, in PRs past, it's generally been unfavorable viewed, and the comprise decided on was to use JSDoc to document the interface. The downside being that you need a common place to put the interface and there isn't a good common spot right now. I suppose I could put it in the embeddable_handler_registery. What do you think of that approach?

cc @weltenwort since I think most of the conversations I had regarding this topic were with you, can you confirm that I'm stating that correctly and you prefer JSDoc interface definitions over abstract classes? Perhaps we should bring this up as a new issue for team wide votes so we can have the decision written down and everyones input.

stacey-gammon · 2017-06-27T18:27:38Z

@thomasneirynck

I'd also add, beyond the 'render', `getEditPath', ... two new methods:
a destroy method, that gets called for all when people navigate away from the dashboard.
a resize(), that instructs an embeddalbe to fit to the dimension of the new surrounding container.

Right now each embeddable handles destroying itself when that happens:

visualizationInstance.on('$destroy', function () {...}

If we wanted to extract it out into a new function in the handler, we'd have to keep track of all panels that were rendered, and map them to their scope objects, because a destroy on a handler will need to destroy every rendered panel.

Maybe what we need, in the long term, is to have and Embeddable and an EmbeddableHandler, and the Embeddable has a destroy method. But as of right now I think that would just complicate the logic.

The same thing goes for the resize function - you'd need a context of all the panels the handler rendered, which right now, it doesn't need to store. The resizing all works naturally, so I'm not even sure when I would call it in dashboard.

stacey-gammon · 2017-06-27T18:29:03Z

@thomasneirynck

For each EmbeddableHandler, consider making name and title constants in the module-scope. These are immutable properties, so they don't need to be instance-properties assigned in the cosntructor.

I got rid of title for now, it wasn't being used. I'll bring it back when it is. As for name, I'm pretty sure I need to leave it as is for the registry handlers to work correctly:
const embeddableHandler = embeddableHandlers.byName[$scope.panel.type];

…onality into a new class

…dable-handlers

…anel

stacey-gammon · 2017-08-03T14:48:33Z

@spalger -- any more feedback on your end?

spalger

Looking good, some more feedback

spalger · 2017-08-03T19:17:41Z

src/ui/public/embeddable/embeddable_handlers_registry.js

@@ -0,0 +1,9 @@
+import { uiRegistry } from 'ui/registry/_registry';


Can we put this in ui/registry with the rest of the registries? I'm not a huge fan of the decision to go that route but we should probably stay consistent.

We are already starting to deviate elsewhere - savedObjectsRegistery. Though yes most are in the ui/registry folder. If you feel strongly about it I'll move it over to ui/registry, though I'd rather not. :)

spalger · 2017-08-03T19:18:40Z

src/ui/public/embeddable/container_api.js

+   * @param operator
+   * @param index
+   */
+  onFilter(/*field, value, operator, index */) {


This should probably be addFilter() right?

spalger · 2017-08-03T19:22:31Z

src/ui/public/embeddable/container_api.js

+   * @param uiState {Object} - the uiState for the child.
+   * @returns {Object}
+   */
+  createChildUiState(/* path, uiState */) {


Can we call this initalState or something that disambiguates it from the PersistedState instances we usually call uiState? If it was a PersistedState then we would just be able to call createChild() on it.

spalger · 2017-08-03T19:29:25Z

src/ui/public/embeddable/container_api.js

+/**
+ * The ContainerAPI is an interface for embeddable objects to interact with the container they are embedded within.
+ */
+export class ContainerAPI {


spalger · 2017-08-03T19:33:32Z

src/core_plugins/kibana/public/discover/embeddable/search_embeddable_handler.js

+    return savedObject.title;
+  }
+
+  async render(domNode, panel, container) {


Unfortunately there are a lot of unexpected and hard-to-debug issues that pop-up when native promises or async/await with Angular. I hate to say it but please convert any async/await or native promise usage that is interacting with Angular to use Angular promises.

so saaaaaaad :(

spalger · 2017-08-03T19:35:58Z

src/ui/public/embeddable/embeddable_handler.js

+   * from panel.id because you can have the same object rendered multiple times in two different panels.
+   * @param {Promise.<void>} A promise that resolves when the object is finished rendering.
+   */
+  async render(/* domNode, panel, container */) {


Looks like the params here are now domNode, panel, containerApi? jsdocs need updates.

spalger · 2017-08-03T19:40:09Z

src/core_plugins/kibana/public/discover/embeddable/search_embeddable_handler.js

+  }
+
+  async getEditPath(panelId) {
+    return await this.searchLoader.urlFor(panelId);


If an embeddable had controls for editing itself it would need to be able to tell consumers that the title/path/etc. have changed, any idea how that would work?

I assume we couldn't just replace these methods with something like ContainerAPI#setEditPath() and ContainerAPI#setTitle() because we want to show the edit path and title for a panel without rendering it... Maybe the container API could expose a method to invalidate the editPath and titles that are cached by containers?

Yea, that's a good point. Not sure how I feel about adding that functionality now though when it's not needed by anything we build. Yes, we want this to be a flexible embeddable system for others to easily write panels in, but I think during this initial phase, it's okay to limit what the container api exposes. We should probably add some kind of forceRefresh functionality that forces a rebuild of the whole panel at some point. Just not sure I want to go down this path without an easy way to test it works. Thoughts?

Yeah, without a use case we probably shouldn't design a solution, I'm fine with punting for now. We can figure out how to make it work when we need it (hint: I'm thinking about observables)

- Rename onFilter to addFilter - Use angular promises instead of async/await - fix jsdoc - rename createChildUiState to getInitialState

spalger · 2017-08-03T21:29:48Z

src/core_plugins/kibana/public/dashboard/dashboard_container_api.js

+    return this.dashboardState.getIsViewMode();
+  }
+
+  getInitialState(path, uiState) {


Haha, I wasn't very clear, I was asking for createChildUiState(path, initialState)

This method returns an instance of PersistedState, which we usually refer to as uiState. This method also takes an argument which it calls uiState, but it's actually just a plain object that is used as the initial state for the uiState that is returned.

hah, okay, yea, this function is carried over from the past and I never understood much of it. Will switch to your intended version. :)

spalger · 2017-08-03T21:32:47Z

src/ui/public/embeddable/embeddable_handler.js

+   * @return {Promise.<string>} a promise that resolves with the path that dictates where the user will be navigated to
+   * when they click the edit icon.
+   */
+  async getEditPath(/* panelId */) {


No need for async (or Promise.reject below)

spalger · 2017-08-03T21:49:32Z

Looks like the spy panel isn't visible on the dashboard anymore:

master:

pr:

…dable-handlers

spalger · 2017-08-03T21:54:41Z

Looks like spy panel shows up on visualizations that are added, sounds like a two-way binding is being broken

stacey-gammon · 2017-08-03T22:09:57Z

Nice find. I see where it's coming from. Strange though because I have a test that verifies the spyPaneToggle button exists, and works, and that should have caught it.

Will investigate.

There is still a bit of a bug here as mentioned in elastic#13340 but it will be fixed separately as it’s also an issue in master

stacey-gammon · 2017-08-04T13:22:36Z

Added a test that would have caught it and fixed up the logic. The binding part is still an issue, but it's an issue in master as well and I filed #13340 to keep track. It's just much less obvious of a bug when the logic is correct because it only hides the spy pane in embed mode and you aren't supposed to leave embed mode without a hard refresh.

…dable-handlers

* Move dashboard panel rendering logic to each registered type. * Remove dashboard knowledge of click and brush handlers for visualizations Move it to the VisualizeEmbeddableHandler. * merge master with manual changes * No need to use lodash * Add EmbeddableHandler base class * Use correct path to embeddable_handlers_registry * clean up * Set visualize scope uiState that is of type PersistedState, otherwise it won't actually be set. * add retry to loading saved search data * Fix handleError param and remove unnecessary private param * Rename savePanelState updatePanel and return the new object rather than mutating the original * Make ContainerAPI a base class and move the dashboard specific functionality into a new class * Make api's async and clean up documentation * Fix panel tests * Fix bug which broke tests - need to pass container-api to dashboard-panel * Address code comments - Rename onFilter to addFilter - Use angular promises instead of async/await - fix jsdoc - rename createChildUiState to getInitialState * changed the wrong variable name * no need for async or Promise.reject on interface * add tests that will fail due to spy pane issue in this PR * Fix logic with spy pane toggle There is still a bit of a bug here as mentioned in elastic#13340 but it will be fixed separately as it’s also an issue in master * Fix failing test

* Move dashboard panel rendering logic to each registered type. * Remove dashboard knowledge of click and brush handlers for visualizations Move it to the VisualizeEmbeddableHandler. * merge master with manual changes * No need to use lodash * Add EmbeddableHandler base class * Use correct path to embeddable_handlers_registry * clean up * Set visualize scope uiState that is of type PersistedState, otherwise it won't actually be set. * add retry to loading saved search data * Fix handleError param and remove unnecessary private param * Rename savePanelState updatePanel and return the new object rather than mutating the original * Make ContainerAPI a base class and move the dashboard specific functionality into a new class * Make api's async and clean up documentation * Fix panel tests * Fix bug which broke tests - need to pass container-api to dashboard-panel * Address code comments - Rename onFilter to addFilter - Use angular promises instead of async/await - fix jsdoc - rename createChildUiState to getInitialState * changed the wrong variable name * no need for async or Promise.reject on interface * add tests that will fail due to spy pane issue in this PR * Fix logic with spy pane toggle There is still a bit of a bug here as mentioned in #13340 but it will be fixed separately as it’s also an issue in master * Fix failing test

stacey-gammon mentioned this pull request Jun 2, 2017

[POC] Visualize refactor with embeddable #12147

Closed

stacey-gammon force-pushed the embeddable-handlers branch 2 times, most recently from b30fafe to 1602e2e Compare June 2, 2017 18:27

stacey-gammon mentioned this pull request Jun 2, 2017

Push panel rendering logic out of dashboard #11618

Closed

thomasneirynck self-requested a review June 2, 2017 18:37

stacey-gammon force-pushed the embeddable-handlers branch 5 times, most recently from 713fca2 to 2847207 Compare June 5, 2017 15:34

stacey-gammon added :Sharing review v6.1.0 and removed v6.1.0 labels Jun 6, 2017

stacey-gammon requested a review from kimjoar June 6, 2017 16:34

stacey-gammon added the v6.0.0 label Jun 9, 2017

stacey-gammon mentioned this pull request Jun 13, 2017

Nested dashboard panel groups #12313

Closed

kimjoar approved these changes Jun 15, 2017

View reviewed changes

thomasneirynck reviewed Jun 19, 2017

View reviewed changes

stacey-gammon mentioned this pull request Jun 23, 2017

Extract url logic out from chrome.nav internals #12479

Merged

thomasneirynck self-requested a review June 27, 2017 18:57

stacey-gammon mentioned this pull request Jun 29, 2017

Support multiple query languages in the query bar #11915

Closed

stacey-gammon removed the v6.0.0 label Jun 29, 2017

Move dashboard panel rendering logic to each registered type.

0e1e21d

stacey-gammon added 5 commits August 1, 2017 12:39

Make ContainerAPI a base class and move the dashboard specific functi…

0a2545a

…onality into a new class

Make api's async and clean up documentation

3c9fe4e

Fix panel tests

be3afde

Merge branch 'master' of https://github.com/elastic/kibana into embed…

a01b96e

…dable-handlers

Fix bug which broke tests - need to pass container-api to dashboard-p…

6ccedd3

…anel

spalger suggested changes Aug 3, 2017

View reviewed changes

Address code comments

78cce39

- Rename onFilter to addFilter - Use angular promises instead of async/await - fix jsdoc - rename createChildUiState to getInitialState

spalger reviewed Aug 3, 2017

View reviewed changes

stacey-gammon added 2 commits August 3, 2017 17:34

changed the wrong variable name

033d376

no need for async or Promise.reject on interface

4ab7072

Merge branch 'master' of https://github.com/elastic/kibana into embed…

b29f6eb

…dable-handlers

stacey-gammon added 2 commits August 4, 2017 09:14

add tests that will fail due to spy pane issue in this PR

a685b47

Fix logic with spy pane toggle

f74cc0e

There is still a bit of a bug here as mentioned in elastic#13340 but it will be fixed separately as it’s also an issue in master

spalger approved these changes Aug 5, 2017

View reviewed changes

stacey-gammon added 3 commits August 7, 2017 13:51

Merge branch 'master' of https://github.com/elastic/kibana into embed…

6c46134

…dable-handlers

Merge branch 'master' of https://github.com/elastic/kibana into embed…

e401b14

…dable-handlers

Fix failing test

ccfaadb

stacey-gammon merged commit 6e74452 into elastic:master Aug 8, 2017

stacey-gammon mentioned this pull request Aug 8, 2017

[6.x] Create the concept of embeddableHandlers (#12146) #13386

Merged

stacey-gammon deleted the embeddable-handlers branch October 24, 2017 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create the concept of embeddableHandlers #12146

Create the concept of embeddableHandlers #12146

stacey-gammon commented Jun 2, 2017

stacey-gammon commented Jun 6, 2017

stacey-gammon commented Jun 12, 2017

kimjoar left a comment

kimjoar Jun 15, 2017

stacey-gammon commented Jun 15, 2017

thomasneirynck left a comment •

edited

Loading

stacey-gammon commented Jun 19, 2017

stacey-gammon commented Jun 27, 2017

stacey-gammon commented Jun 27, 2017

stacey-gammon commented Aug 3, 2017

spalger left a comment

spalger Aug 3, 2017

stacey-gammon Aug 3, 2017

spalger Aug 3, 2017

spalger Aug 3, 2017

spalger Aug 3, 2017

spalger Aug 3, 2017

stacey-gammon Aug 3, 2017

spalger Aug 3, 2017

spalger Aug 3, 2017

stacey-gammon Aug 3, 2017

spalger Aug 3, 2017 •

edited

Loading

spalger Aug 3, 2017 •

edited

Loading

stacey-gammon Aug 3, 2017

spalger Aug 3, 2017

spalger commented Aug 3, 2017

spalger commented Aug 3, 2017 •

edited

Loading

stacey-gammon commented Aug 3, 2017

stacey-gammon commented Aug 4, 2017

		@@ -0,0 +1,9 @@
		import { uiRegistry } from 'ui/registry/_registry';

Create the concept of embeddableHandlers #12146

Create the concept of embeddableHandlers #12146

Conversation

stacey-gammon commented Jun 2, 2017

stacey-gammon commented Jun 6, 2017

stacey-gammon commented Jun 12, 2017

kimjoar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Jun 15, 2017

thomasneirynck left a comment • edited Loading

Choose a reason for hiding this comment

stacey-gammon commented Jun 19, 2017

stacey-gammon commented Jun 27, 2017

stacey-gammon commented Jun 27, 2017

stacey-gammon commented Aug 3, 2017

spalger 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

spalger Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Aug 3, 2017

spalger commented Aug 3, 2017 • edited Loading

stacey-gammon commented Aug 3, 2017

stacey-gammon commented Aug 4, 2017

thomasneirynck left a comment •

edited

Loading

spalger Aug 3, 2017 •

edited

Loading

spalger Aug 3, 2017 •

edited

Loading

spalger commented Aug 3, 2017 •

edited

Loading