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 Navigation] Register alias with Visualizations plugin #162840

Closed
Tracked by #154354
nickpeihl opened this issue Jul 31, 2023 · 1 comment · Fixed by #170810
Closed
Tracked by #154354

[Dashboard Navigation] Register alias with Visualizations plugin #162840

nickpeihl opened this issue Jul 31, 2023 · 1 comment · Fixed by #170810
Assignees
Labels
Feature:Dashboard Dashboard related features impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Dashboard Navigation Related to the Dashboard Navigation Project Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@nickpeihl
Copy link
Member

We should register the navigation embeddable alias with the Visualizations plugin so that saved objects can appear in the Visualize library.

We register the alias in the public/plugin.ts like this:

plugins.visualizations.registerAlias({
  aliasPath: '/',
  aliasApp: CONTENT_ID,
  name: CONTENT_ID,
  title: APP_NAME,
  icon: APP_ICON,
  description: '', // TODO
  stage: 'experimental' as VisualizationStage,
  appExtensions: {
    visualizations: {
      docTypes: [CONTENT_ID],
      searchFields: ['title^3'],
      toListItem(linkItem: NavigationEmbeddableCrudTypes['Item']) {
        const { id, type, updatedAt, attributes } = linkItem;
        const { title, description } = attributes;

        return {
          id,
          title,
          description,
          updatedAt,
          icon: APP_ICON,
          stage: 'experimental' as VisualizationStage,
          savedObjectType: type,
          typeTitle: APP_NAME,
          editUrl: '/',
        };
      },
    },
  },
});

However, the navigation embeddable does not have its own editor app. The aliasPath is required for applications to register with the visualizations alias registry. Since navigation embeddable does not have a separate editor application, we will need to change how we link to the flyout. We may create an ad-hoc editor application or change the VisTypeAliasRegistry definition to accept a function that opens an editor flyout.

Until we register the alias, the only way to delete Links panels that are saved to the library is via the Management - Saved Objects list view. Links panels saved to the Library can still be added to a Dashboard from the Add panel flyout.

@nickpeihl nickpeihl added loe:large Large Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Project:Dashboard Navigation Related to the Dashboard Navigation Project labels Jul 31, 2023
@botelastic botelastic bot added the needs-team Issues missing a team label label Jul 31, 2023
@nickpeihl nickpeihl added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Jul 31, 2023
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot removed the needs-team Issues missing a team label label Jul 31, 2023
@Heenawter Heenawter changed the title [Navigation embeddable] Register alias with Visualizations plugin [Dashboard Navigation] Register alias with Visualizations plugin Aug 8, 2023
Heenawter added a commit that referenced this issue Aug 16, 2023
## 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)
@Heenawter Heenawter added the Feature:Dashboard Dashboard related features label Aug 31, 2023
@Heenawter Heenawter self-assigned this Oct 3, 2023
@Heenawter Heenawter added loe:medium Medium Level of Effort and removed loe:large Large Level of Effort labels Nov 7, 2023
kibanamachine pushed a commit to yctercero/kibana that referenced this issue Nov 22, 2023
…0810)

Closes elastic#162840

## Summary

This PR adds a visualization alias for the new Links embeddable so that
all Links library items can be managed/edited from the Visualization
library, like so:


https://github.com/elastic/kibana/assets/8698078/8541506b-cfdd-4a2f-8bc2-841220def7a3

However, in order to get the above working, it was unfortunately not as
simple as just adding a visualization alias. Because the Links
embeddable does not have a dedicated editing app (all editing/creation
is done through a flyout), the usual `aliasPath` + `aliasApp` redirect
that happens for editing an alias did not work in this case.

To get around this, I've had to make changes to how aliases are
registered, as well as both the Visualization `VisualizeListing`
component and the generic `TableListViewTableComp` content management
component:

- **Summary of visualization alias changes:**

First off, I made changes to the typing of aliases - specifically,
rather than taking two independent `aliasPath` and `aliasApp` props,
I've combined them into a singular `alias` prop which will either be of
type `{ alias: string; path: string; }` or `{ embeddableType: string;
}`. This makes it easier to determine (a) whether a visualization is of
type `BaseVisType` or `VisTypeAlias` and (b) if it **is** of type
`VisTypeAlias`, how the editing of that vis should be handled.
Specifically, if `alias` is of type `{ alias: string; path: string; }`,
then it is a normal visualization and behaviour should be the same as it
was previously; however, if it is of type `{ embeddableType: string; }`,
then this is an **inline** alias - i.e. editing should be done inline
via the embeddable factory's edit method.

- **Summary of `VisualizeListing`  changes**

The primary changes here were made to the `editItem` callback -
specifically, if the fetched saved object has neither an `editApp` nor
an `editUrl`, then it will now try to fetch the embeddable factory for
the given saved object type and, if this factory exists, it will call
the `getExplicitInput` method in order to handle editing.

- **Summary of `TableListViewTableComp` changes**

Previously, an error would be thrown if both a `getDetailViewLink` and
an `onClickTitle` prop were provided - while I understand the original
reasoning for adding this catch, this no longer works if we want to
support inline editing like this. In this case, I needed **only** the
Link embeddable items to have an `onClick` while keeping the behaviour
for other visualizations the same (i.e. all other visualization types
should continue to link off to their specific editor apps) - however,
since this method is not provided **per item**, I had no way of making
an exception for just one specific item type.

Therefore, to get around this, it is now considered to be valid for
**both** the `getDetailViewLink` and `onClickTitle` props to be defined
for the `TableListViewTableComp` component. In order to prevent conflict
between the two props, I have made it so that, if both are provided,
`getDetailViewLink` **always takes precedence** over `onClickTitle` - in
this case, `onClickTitle` will **only** be called if `getDetailViewLink`
returns `undefined` for a given item. I have added a comment to
hopefully make this clear for consumers.

### 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
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Dashboard Navigation Related to the Dashboard Navigation Project Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants