Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Embeddable Rebuild] Support grouping in ADD_PANEL_TRIGGER #179872

Merged
merged 9 commits into from
Apr 5, 2024
Merged

[Embeddable Rebuild] Support grouping in ADD_PANEL_TRIGGER #179872

merged 9 commits into from
Apr 5, 2024

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Apr 2, 2024 •
edited
Loading

Summary

Resolves #179565

Adds support for the grouping property in actions assigned to the ADD_PANEL_TRIGGER trigger.

In the "Add panel" context menu, groups from the embeddable factories and ADD_PANEL_TRIGGER actions are merged, allowing us to group legacy embeddables with the react ones.

Checklist

@darnautov darnautov self-assigned this Apr 2, 2024
@darnautov
Copy link
Contributor Author

/ci

@darnautov darnautov added release_note:skip Skip the PR/issue when compiling release notes Feature:Embeddables Relating to the Embeddable system project:embeddableRebuild v8.14.0 labels Apr 2, 2024
@darnautov darnautov marked this pull request as ready for review April 3, 2024 13:15
@darnautov darnautov requested a review from a team as a code owner April 3, 2024 13:15
@darnautov darnautov requested review from nreese and ThomThomson April 3, 2024 13:16
@darnautov darnautov added the Feature:Dashboard Dashboard related features label Apr 3, 2024
@kibanamachine kibanamachine added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Apr 3, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@darnautov darnautov added impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:medium Medium Level of Effort labels Apr 3, 2024
ThomThomson
ThomThomson reviewed Apr 3, 2024
View reviewed changes
Copy link
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.

This works great! Tested in chrome and the groupings were applied as expected.

I'm wondering though, if you would do another pass on DX for this change? It seems to me that this file is (and has been) in need of a bit of an organizational cleanup to lower the number of lines + the cognitive load.

The jest tests are looking great also, and I see you've added coverage for all the new functionality. Thanks for that!

src/plugins/dashboard/public/dashboard_app/top_nav/add_panel_action_menu_items.test.ts
@@ -21,8 +21,40 @@ describe('getAddPanelActionMenuItems', () => {
isCompatible: () => Promise.resolve(true),
execute: jest.fn(),
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This new test case is great! Really good test coverage in this PR as a whole.

src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx
};
};

export const mergeGroupedItemsProvider =
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason these need to be curried functions? Could it be simpler with all arguments taken in one function call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to separate the dependencies of the function and the input arguments. I don't have strong preferences here, so whatever pattern the team prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

We usually avoid curried functions unless they're necessary, but it's completely up to you. I consider this file to mainly be temporary as we have a cleanup followup issue.

src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx
@@ -41,6 +46,97 @@ interface UnwrappedEmbeddableFactory {
isEditable: boolean;
}

export type GetEmbeddableFactoryMenuItem = ReturnType<typeof getEmbeddableFactoryMenuItemProvider>;

export const getEmbeddableFactoryMenuItemProvider =
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a separate function? I see it's exported for test coverage, but I suspect it might be more robust to test this editor menu as a React component via @testing-library/react

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I made it a separate function to test the functionality I added.
It's indeed can be a jest test with @testing-library/react, but there were no tests for this file whatsoever. I want to keep changes to a minimum to unblock #179547

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, it should be covered with functional tests using FTR. I plan to add some in #179547, because first it needs this PR to get into main.

Copy link
Contributor

Choose a reason for hiding this comment

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

This strategy sounds good to me. These tests provide good coverage for the time being.

@darnautov darnautov requested review from mgiota and ThomThomson April 4, 2024 09:24
ThomThomson
ThomThomson approved these changes Apr 4, 2024
View reviewed changes
Copy link
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.

Approving to unblock - even though the file is still quite messy, it's more important to move forward for the time being Likely we can do this cleanup I'm imagining once we remove the legacy Embeddable system. I've opened a followup issue here.

src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx
@@ -41,6 +46,97 @@ interface UnwrappedEmbeddableFactory {
isEditable: boolean;
}

export type GetEmbeddableFactoryMenuItem = ReturnType<typeof getEmbeddableFactoryMenuItemProvider>;

export const getEmbeddableFactoryMenuItemProvider =
Copy link
Contributor

Choose a reason for hiding this comment

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

This strategy sounds good to me. These tests provide good coverage for the time being.

src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx
};
};

export const mergeGroupedItemsProvider =
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually avoid curried functions unless they're necessary, but it's completely up to you. I consider this file to mainly be temporary as we have a cleanup followup issue.

mgiota
mgiota approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

LGTM!

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

@darnautov darnautov enabled auto-merge (squash) April 5, 2024 11:38
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
dashboard 393.3KB 394.0KB +648.0B

History

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

cc @darnautov

@darnautov darnautov merged commit bacad3a into elastic:main Apr 5, 2024
16 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 5, 2024
@thomheymann thomheymann mentioned this pull request Apr 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomThomson ThomThomson ThomThomson approved these changes

@mgiota mgiota mgiota approved these changes

@nreese nreese Awaiting requested review from nreese

Assignees

@darnautov darnautov

Labels
backport:skip This commit does not require backporting Feature:Dashboard Dashboard related features Feature:Embeddables Relating to the Embeddable system 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 v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Embeddable Rebuild] Support grouping in ADD_PANEL_TRIGGER
6 participants
@darnautov @elasticmachine @kibana-ci @mgiota @ThomThomson @kibanamachine