Skip to content

[Embeddable Rebuild] [Image] Migrate image embeddable to new embeddable framework#178544

Merged
Heenawter merged 51 commits intoelastic:mainfrom
Heenawter:refactor-image-embeddable_2024-03-08
Apr 8, 2024
Merged

[Embeddable Rebuild] [Image] Migrate image embeddable to new embeddable framework#178544
Heenawter merged 51 commits intoelastic:mainfrom
Heenawter:refactor-image-embeddable_2024-03-08

Conversation

@Heenawter
Copy link
Copy Markdown
Contributor

@Heenawter Heenawter commented Mar 12, 2024

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:

  1. I fixed the contrast of the optionsMenuButton as described in [Embeddable Rebuild] [Image] Migrate image embeddable to new embeddable framework #178544 (comment)

  2. I ensured that the PresentationPanel enforces 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):

    Before After
    View mode image image
    Edit mode image image

Checklist

For maintainers

@Heenawter Heenawter added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// project:embeddableRebuild labels Mar 12, 2024
@Heenawter Heenawter self-assigned this Mar 12, 2024
@Heenawter Heenawter force-pushed the refactor-image-embeddable_2024-03-08 branch from 3f562e4 to 7a7dc50 Compare March 12, 2024 22:27
@Heenawter
Copy link
Copy Markdown
Contributor Author

/ci

1 similar comment
@Heenawter
Copy link
Copy Markdown
Contributor Author

/ci

@Heenawter Heenawter force-pushed the refactor-image-embeddable_2024-03-08 branch from 83580af to 97d6261 Compare March 13, 2024 17:13
@Ikuni17
Copy link
Copy Markdown
Contributor

Ikuni17 commented Mar 13, 2024

/ci

1 similar comment
@Heenawter
Copy link
Copy Markdown
Contributor Author

/ci

@Heenawter Heenawter force-pushed the refactor-image-embeddable_2024-03-08 branch from e727f90 to c5af8e6 Compare March 19, 2024 17:45
@Heenawter Heenawter changed the title [Embeddable Refactor] [Image] Migrate image embeddable to new embeddable framework [Embeddable Rebuild] [Image] Migrate image embeddable to new embeddable framework Mar 20, 2024
Heenawter added a commit that referenced this pull request Mar 21, 2024
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>
@Heenawter Heenawter force-pushed the refactor-image-embeddable_2024-03-08 branch 2 times, most recently from 5863011 to 08edf37 Compare March 22, 2024 13:59
@Heenawter
Copy link
Copy Markdown
Contributor Author

/ci

3 similar comments
@Heenawter
Copy link
Copy Markdown
Contributor Author

/ci

@Heenawter
Copy link
Copy Markdown
Contributor Author

/ci

@Heenawter
Copy link
Copy Markdown
Contributor Author

/ci

@Heenawter Heenawter force-pushed the refactor-image-embeddable_2024-03-08 branch from 052d961 to aee6b99 Compare March 25, 2024 19:11
@Heenawter
Copy link
Copy Markdown
Contributor Author

/ci

@Heenawter
Copy link
Copy Markdown
Contributor Author

Unblocked by #179667 and b9c6184

@Heenawter Heenawter requested review from ThomThomson and removed request for ThomThomson April 5, 2024 14:23
Copy link
Copy Markdown
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

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!

Before
Screenshot 2024-04-05 at 3 46 31 PM

After
Screenshot 2024-04-05 at 3 46 53 PM

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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice attention to detail here!

const embeddable = buildApi(
{
...titlesApi,
...(dynamicActionsApi?.dynamicActionsApi ?? {}),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +56 to +61
export interface ReactEmbeddableDynamicActionsApi {
dynamicActionsApi: HasDynamicActions;
dynamicActionsComparator: StateComparators<DynamicActionsSerializedState>;
serializeDynamicActions: () => DynamicActionsSerializedState;
startDynamicActions: () => { stopDynamicActions: () => void };
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a really clean pattern. Initialize & start are two separate things, and start returns stop. Good way of designing this.

@botelastic botelastic bot added the Feature:Drilldowns Embeddable panel Drilldowns label Apr 8, 2024
@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
imageEmbeddable 78 131 +53

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
embeddableEnhanced 23 19 -4
imageEmbeddable 3 1 -2
total -6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
imageEmbeddable 50.3KB 65.0KB +14.7KB
presentationPanel 8.0KB 8.0KB +31.0B
total +14.8KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
embeddableEnhanced 1 2 +1
imageEmbeddable 1 0 -1
total -0

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
embeddableEnhanced 10.7KB 10.7KB +90.0B
imageEmbeddable 8.9KB 5.8KB -3.1KB
presentationPanel 41.9KB 42.1KB +144.0B
total -2.9KB
Unknown metric groups

API count

id before after diff
embeddableEnhanced 23 19 -4
imageEmbeddable 3 1 -2
total -6

async chunk count

id before after diff
imageEmbeddable 5 7 +2

References to deprecated APIs

id before after diff
imageEmbeddable 3 1 -2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Heenawter

@Heenawter Heenawter merged commit 826f7cb into elastic:main Apr 8, 2024
@Heenawter Heenawter deleted the refactor-image-embeddable_2024-03-08 branch April 8, 2024 18:08
@kibanamachine kibanamachine added v8.14.0 backport:skip This PR does not require backporting labels Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Drilldowns Embeddable panel Drilldowns impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:medium Medium Level of Effort project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.14.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Image Embeddable] "Use link" empty state has a layout issue [Embeddables Rebuild] Migrate Image Image Embeddable visual improvements

10 participants