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.

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

[Dashboard] Use factory's displayName in save modal #164105

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Aug 16, 2023

Summary

As part of a larger navigation embeddable discussion, it was noted that there were inconsistencies between how we display the panel type between the saved object modal and the context menu action. Turns out, this is because we were using the embeddable type in the "Save to library" modal title while the context menu action uses the factory's displayName - while this coincidentally worked in most cases (maps and visualization), it does not work for Lens panels, since we want to use the proper noun "Lens" in this case rather than the saved object type "lens":

This was even more obvious with the navigation embeddable, since the saved object type is currently "navigation_embeddable" (this may change in the future):

Therefore, by switching to use the factory's displayName (if available), the saved object modal will now stay consistent with the "Edit <x>" option in the context menu for each panel.

Before After
image image
image image
image image

No change, as expected.

image image

No change, as expected.

Note that, in the above "After" screenshots, Links is still capitalized unnecessarily - this is because, if we change the factory's displayName to be the lowercase "links", this fixes the modal but it also impacts the item in the add panel context menu and the panel actions context menu, like so:

Add panel context menu Panel actions context menu
image1 Screenshot 2023-08-16 at 1 36 14 PM

This is because the link embeddable is not currently registered as a "visualization" and so the "Add panel" context menu logic is handled as part of getEmbeddableFactoryMenuItem in ./src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx rather than getVisTypeMenuItem (this could potentially be fixed by #162840 since we have custom logic for displaying aliases). While the lowercase "links" in the panel actions context menu is desired, it is not desirable to have the lowercase "links" in the add panel context menu - however, it is not currently possible to change one without changing the other.

Note that this is also true for the "Image" embeddable - by changing the displayName of the image factory from "Image" to "image", the "Edit" panel actions context menu item would be displayed correctly as "Edit image" (currently, it is "Edit Image") but the lowercase "image" would also be displayed in the add panel context menu, which is undesirable.

It will require a larger refactor to fix these inconsistencies, so it should be addressed separately.

Checklist

For maintainers

@Heenawter Heenawter added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. backport:skip This commit does not require backporting labels Aug 16, 2023
@Heenawter Heenawter self-assigned this Aug 16, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id before after diff
embeddable 64.7KB 64.9KB +139.0B

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

cc @Heenawter

@Heenawter Heenawter marked this pull request as ready for review August 16, 2023 19:42
@Heenawter Heenawter requested a review from a team as a code owner August 16, 2023 19:42
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Aug 16, 2023
@Heenawter Heenawter merged commit 0d74544 into elastic:main Aug 16, 2023
@Heenawter Heenawter deleted the fix-save-modal-title_2023-08-16 branch August 16, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Embedding Embedding content via iFrame impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants