[Embeddable Rebuild] [Image] Migrate image embeddable to new embeddable framework#178544
Conversation
3f562e4 to
7a7dc50
Compare
|
/ci |
1 similar comment
|
/ci |
83580af to
97d6261
Compare
|
/ci |
1 similar comment
|
/ci |
e727f90 to
c5af8e6
Compare
Closes #178742 ## Summary In the old embeddable system, when an embeddable is created through its factory, it is "enhanced" via `enhanceEmbeddableWithDynamicActions` in `EmbeddableEnhancedPlugin` - this enhancement is responsible for adding the pieces necessary (for example, the `DynamicActionManager`) to the embeddable to manage drilldowns. In the new system, we are no longer using these class-based factories - so there is no longer a way to enhance embeddables on creation. Therefore, this PR does three main things: 1. It adds a new method `initializeDynamicActions` to the `EnhancedEmbeddablePlugin` and returns it through its start contract (in order to bypass limitations around importing stuff from `xpack`) - this method can be used to enhance all **React-based** embeddables with the stuff necessary for drilldowns to work. As part of this, I had to add a few extra pieces to the `HasDynamicActions` API: - `dynamicActionsState$`: This is a publishing subject that will keep track of the events currently attached to the given embeddable (`DynamicActionsState`) - `setDynamicActions`: A setter for the above publishing subject 2. It modifies all actions tied to dashboard drilldowns - `PanelNotificationsAction`, `EmbeddableToDashboardDrilldown`, `FlyoutEditDrilldownAction`, and `FlyoutCreateDrilldownAction`. 3. It modifies the old `enhanceEmbeddableWithDynamicActions` to add the publishing subject + setter from (1) above to all **legacy** embeddables when they are enhanced - that way, they will still pass the `apiHasDynamicActions` type check + all modified actions continue to work for legacy embeddables. **How to Test** To test with a React embeddable, you can checkout the [draft PR](#178544) containing the new image embeddable. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
5863011 to
08edf37
Compare
|
/ci |
3 similar comments
|
/ci |
|
/ci |
|
/ci |
052d961 to
aee6b99
Compare
|
/ci |
ThomThomson
left a comment
There was a problem hiding this comment.
It's amazing to see the new framework we've worked so hard on used for a real-life Embeddable.
I tested this locally, changing images / colours / drilldowns, resetting and ensuring everything works as expected. I also did a little bit of an investigation on the browser performance.
I did a hard refresh on the Dashboard listing page in main and this branch and did a performance profile on opening a Dashboard for the first time with ~20 image Embeddables. As expected, the time is roughly the same (probably most of the time was spent downloading async bundles) but I'm happy to report that at least on my machine the number of dropped / partially rendered frames has been cut in half! Nice!
This is a really exciting PR, and the organization and cleanliness on display here makes me excited for the future of this architecture. Great work and LGTM!
Left a few comments and nits, but mostly just conversation pieces.
| * If available, the parent API will keep track of which flyout is open and close it | ||
| * if the app changes, disable certain actions when the flyout is open, etc. | ||
| */ | ||
| const overlayTracker = tracksOverlays(parentApi) ? parentApi : undefined; |
There was a problem hiding this comment.
Nice attention to detail here!
src/plugins/image_embeddable/public/components/image_embeddable.tsx
Outdated
Show resolved
Hide resolved
src/plugins/image_embeddable/public/components/image_embeddable.tsx
Outdated
Show resolved
Hide resolved
src/plugins/image_embeddable/public/components/image_embeddable.tsx
Outdated
Show resolved
Hide resolved
| const embeddable = buildApi( | ||
| { | ||
| ...titlesApi, | ||
| ...(dynamicActionsApi?.dynamicActionsApi ?? {}), |
There was a problem hiding this comment.
nit: I think we need a better naming convention for these packages that contain an api, comparators, etc. to avoid situations like this: api.api...
We can't call them services or modules or components or apis because those are all taken. @elastic/kibana-presentation any ideas?
There was a problem hiding this comment.
Yup, dynamicActionsApi?.dynamicActionsApi was definitely painful to write 😆 interface? framework? I dunno... API feels like the correct word despite the duplication 🙈 If it was dynamicActionsApi?.api it might not be so bad...
There was a problem hiding this comment.
We could always leave the word off entirely if we can't think of anything? Like dynamicActions.api. That's kinda the pattern we use today, I think titles at least works this way.
| export interface ReactEmbeddableDynamicActionsApi { | ||
| dynamicActionsApi: HasDynamicActions; | ||
| dynamicActionsComparator: StateComparators<DynamicActionsSerializedState>; | ||
| serializeDynamicActions: () => DynamicActionsSerializedState; | ||
| startDynamicActions: () => { stopDynamicActions: () => void }; | ||
| } |
There was a problem hiding this comment.
This is a really clean pattern. Initialize & start are two separate things, and start returns stop. Good way of designing this.
…nawter/kibana into refactor-image-embeddable_2024-03-08
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @Heenawter |


Closes #174962
Closes #165848
Closes #179521
Summary
This PR converts the Image embeddable to the new React embeddable framework. There should not be any changes in user-facing behaviour (unless they were made intentionally, such as the small change described here) - therefore, testing of this PR should be focused on ensuring that no behaviour is changed and/or broken with this refactor.
Since I was already doing a major overhaul, I took the opportunity to clean up some of the image embeddable code, such as the small change described here. Some of my changes are heavily influenced by the Presentation team style (such as my changes to the file organization) so, if there are any disagreements, I am 100% open to make changes - after all, this code does not belong to us and we are not responsible for maintenance. Since this is the first embeddable to be officially refactored (🎉), I expect there to be lots of questions + feedback and that is okay!
Small Style Changes
In order to close #165848, I did two things:
I fixed the contrast of the
optionsMenuButtonas described in [Embeddable Rebuild] [Image] Migrate image embeddable to new embeddable framework #178544 (comment)I ensured that the
PresentationPanelenforces rounded corners in view mode while keeping appearances consistent in edit mode (i.e. the upper corners remain square so that it looks consistent with the title bar):Checklist
For maintainers