Skip to content

Commit

Permalink
[Dashboard] Use factory's displayName in save modal (#164105)
Browse files Browse the repository at this point in the history
## Summary

As part of a [larger navigation embeddable
discussion](#162285 (comment)),
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"`:

<p align="center"><img width="500"
src="https://github.com/elastic/kibana/assets/8698078/d56659bf-ff13-4974-80bb-f267fdcb1cff"/></p>

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

<p align="center"><img width="500"
src="https://github.com/elastic/kibana/assets/8698078/868daebb-fa51-4f97-bd23-3cf47472fadf"/></p>



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](https://github.com/elastic/kibana/assets/8698078/d56659bf-ff13-4974-80bb-f267fdcb1cff)
|
![image](https://github.com/elastic/kibana/assets/8698078/eb651928-3e9d-4c7c-bc3a-d6e3e98854f6)
|
|
![image](https://github.com/elastic/kibana/assets/8698078/868daebb-fa51-4f97-bd23-3cf47472fadf)
|
![image](https://github.com/elastic/kibana/assets/8698078/d1f9ca7e-1711-4041-b025-efb2b36f0684)|
|
![image](https://github.com/elastic/kibana/assets/8698078/633f8d6b-185f-4b47-b9ca-4bda24344c44)
|
![image](https://github.com/elastic/kibana/assets/8698078/8537bf39-a04e-4492-9d20-3162396fc4e2)<br><p
align="center">**No change**, as expected.</p> |
|
![image](https://github.com/elastic/kibana/assets/8698078/75a19c15-efec-4c32-b980-6200d7c17e8e)
|
![image](https://github.com/elastic/kibana/assets/8698078/bd7cb2e9-ae4f-4b52-a7ef-04d74949fd28)<br><p
align="center">**No change**, as expected.</p> |

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](https://github.com/elastic/kibana/assets/8698078/57e16f0b-1f5c-4291-afd3-a69e57540967)
| ![Screenshot 2023-08-16 at 1 36 14
PM](https://github.com/elastic/kibana/assets/8698078/34d41119-4c84-460d-923c-1d2570dadfd9)|

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

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)

### 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)
  • Loading branch information
Heenawter authored Aug 16, 2023
1 parent 2d2975f commit 0d74544
Showing 1 changed file with 6 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ export class AttributeService<
RefType extends SavedObjectEmbeddableInput = SavedObjectEmbeddableInput,
MetaInfo extends unknown = unknown
> {
private embeddableFactory;

constructor(
private type: string,
private toasts: NotificationsStart['toasts'],
Expand All @@ -75,6 +77,7 @@ export class AttributeService<
if (!factory) {
throw new EmbeddableFactoryNotFoundError(this.type);
}
this.embeddableFactory = factory;
}
}

Expand Down Expand Up @@ -186,7 +189,9 @@ export class AttributeService<
(input as ValType)[ATTRIBUTE_SERVICE_KEY].title
)}
showCopyOnSave={false}
objectType={this.type}
objectType={
this.embeddableFactory ? this.embeddableFactory.getDisplayName() : this.type
}
showDescription={false}
/>
);
Expand Down

0 comments on commit 0d74544

Please sign in to comment.