Migrate share registry #50137
Migrate share registry #50137
Conversation
flash1293
added
release_note:plugin_api_changes
💔 Build Failed |
There was a problem hiding this comment.
Thanks for starting on this -- I took a look at it today, and overall this makes sense to me. TBH I am not sure why we had listed kibana_react as a destination for this functionality in our migration meta issue. Presumably because there was an idea that some of that static components could be shared from there? I prefer your approach of exposing the minimal possible API surface area as a start, especially as this is likely to evolve during migration still. (For example, there's the url shortening REST API src/legacy/server/url_shortening, which still hasn't been migrated and we will eventually need a plan for).
The only thing I'm going back and forth on is whether share is a wide enough domain to justify a standalone plugin. Since this functionality doesn't neatly fit as a service inside of one our existing plugins, I think this makes sense for the time being. I'll mull this over a little bit, and will also do some testing on this PR tomorrow.
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
Jenkins, test this |
|
Marking this ready for review because I'm 99% certain the build failures are just infrastructure/flaky test ones at this point. |
💚 Build Succeeded |
There was a problem hiding this comment.
Tested locally (Chrome OSX) and everything LGTM! Thanks for doing this.
One favor to ask - Would you mind also adding ui/share to the table in MIGRATION.md so we have its new location documented there too?
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
|
|
||
| this.isOpen = true; | ||
|
|
||
| document.body.appendChild(this.container); |
There was a problem hiding this comment.
I was going to say that direct document access should probably be avoided in plugins in favor of core's rootDomElement, but it seems we are not actually exposing it. What are the recommended guidelines for this @joshdover ? As this seems quite common, should we provide an API for plugins to create that kind of containers?
|
@elasticmachine merge upstream |
|
@cchaos Can you take another look? I removed the unnecessary classes and it looks all good AFAICT. |
💚 Build Succeeded |
This PR moves the share registry and surrounding functionality into a separate Kibana platform plugin.
Dev docs
The
ui/shareregistry is removed in favor of theshareplugin which exposes aregistermethod in the setup contract. The interface of share menu providers does not change except for the removal of angular dependency injection. The function to render the menu also moves into a function exposed by theshareplugin in the start phase instead of a function which has to be called with the menu item providers. The following items have also been renamed:ShowProps->ShowShareMenuOptionsShareMenuItemProps->ShareContextshowShareContextMenu->toggleShareContextMenu