[Dashboard Navigation] Add Links to Visualization library #170810

Heenawter · 2023-11-07T22:09:49Z

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:

Screen.Recording.2023-11-14.at.10.27.50.AM.mov

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

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Heenawter · 2023-11-14T21:35:57Z

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

      a === b ? 0 : a ? -1 : 1
-  );
+    )
+    .filter(({ disableCreate }: VisTypeAlias) => !disableCreate);


Since we are relying on the embeddable factory create method for the link embeddable, we need to hide the visualization alias item from the dropdown.

Good workaround. I'm thinking that as part of the Embeddable refactor, we should introduce a separate registry for creation buttons in the Dashboard top nav. That would make this more explicit and unified.

Heenawter · 2023-11-14T21:36:31Z

src/plugins/links/public/editor/open_editor_flyout.tsx

+    const closeEditorFlyout = (editorFlyout: OverlayRef) => {
+      if (overlayTracker) {
+        overlayTracker.clearOverlays();
+      } else {
+        editorFlyout.close();
+      }
+    };


We can no longer assume that the overlayTracker (from the Dashboard app) exists, since this flyout can now be opened outside of the Dashboard app - therefore, if the overlayTracker does not exist, we need to rely on the normal flyout close method.

Heenawter · 2023-11-14T21:36:41Z

src/plugins/links/public/embeddable/links_embeddable_factory.ts

@@ -128,8 +128,6 @@ export class LinksFactoryDefinition
    initialInput: LinksInput,
    parent?: DashboardContainer
  ): Promise<LinksEditorFlyoutReturn> {
-    if (!parent) return { newInput: {} };


Similar to https://github.com/elastic/kibana/pull/170810/files#r1393019545, since the Link embeddable can now be edited outside of the Dashboard app, it is no longer considered invalid for a Link embeddable to not have a parent.

Note that this early return was actually unnecessary in the first place, because we already treat the parent as potentially undefined in the rest of the code - so, it's safe to remove.

src/plugins/visualizations/public/vis_types/vis_type_alias_registry.ts

src/plugins/visualizations/public/wizard/new_vis_modal.tsx

elasticmachine · 2023-11-14T22:06:14Z

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

ThomThomson

Presentation team changes LGTM! Tested locally in chrome and looked through the code. Nice work getting the title click and the edit button to open the links editor.

ThomThomson · 2023-11-15T16:51:04Z

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

      a === b ? 0 : a ? -1 : 1
-  );
+    )
+    .filter(({ disableCreate }: VisTypeAlias) => !disableCreate);


Good workaround. I'm thinking that as part of the Embeddable refactor, we should introduce a separate registry for creation buttons in the Dashboard top nav. That would make this more explicit and unified.

vadimkibana

Shared UX code changes LGTM

drewdaemon

I like the direction this is going! Event annotations feature a similar library flyout editing experience.

But, why not divorce the Visualize app from the embeddable system?

I can see that the visualize app currently depends on the embeddable plugin, but the only usage is the one in this PR. I'd prefer we make the alias API generic and remove the visualize dependency on embeddables completely.

WDYT?

src/plugins/visualizations/public/vis_types/vis_type_alias_registry.ts

drewdaemon

Looks great. Thank you.

kibana-ci · 2023-11-22T16:25:01Z

💛 Build succeeded, but was flaky

Failed CI Steps

FTR Configs #59

Metrics [docs]

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
`visualizations`	807	804	-3

Async chunks

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

id	before	after	diff
`canvas`	1013.8KB	1013.8KB	+59.0B
`dashboard`	377.2KB	377.2KB	-32.0B
`eventAnnotationListing`	196.9KB	196.8KB	-89.0B
`filesManagement`	90.0KB	89.9KB	-89.0B
`graph`	388.2KB	388.2KB	-89.0B
`links`	25.2KB	25.3KB	+120.0B
`maps`	2.9MB	2.9MB	-89.0B
`visualizations`	268.1KB	268.1KB	+66.0B
total			-143.0B

Page load bundle

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

id	before	after	diff
`lens`	37.2KB	37.2KB	+7.0B
`links`	33.0KB	33.6KB	+658.0B
`maps`	46.7KB	46.7KB	+7.0B
`visualizations`	57.1KB	57.1KB	+9.0B
total			+681.0B

Unknown metric groups

API count

id	before	after	diff
`visualizations`	837	835	-2

History

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

cc @Heenawter

Heenawter self-assigned this Nov 7, 2023

Heenawter force-pushed the links-register-alias_2023-10-17 branch 7 times, most recently from f5f9582 to 6e58933 Compare November 10, 2023 23:02

Heenawter added 4 commits November 14, 2023 08:32

First draft at editing from visualization listing page

bc227bd

Disable hard app link in listing page

e0f60ed

Clean up

ed143f9

Make title clickable for Link type

89b806a

Heenawter force-pushed the links-register-alias_2023-10-17 branch from 6e58933 to 89b806a Compare November 14, 2023 15:32

Heenawter added 2 commits November 14, 2023 12:51

Close flyout on navigation

3c4ba63

Cleaner implementation

40970bf

Heenawter commented Nov 14, 2023

View reviewed changes

src/plugins/visualizations/public/vis_types/vis_type_alias_registry.ts Outdated Show resolved Hide resolved

Heenawter commented Nov 14, 2023

View reviewed changes

src/plugins/visualizations/public/wizard/new_vis_modal.tsx Outdated Show resolved Hide resolved

Heenawter marked this pull request as ready for review November 14, 2023 22:06

Heenawter requested review from a team as code owners November 14, 2023 22:06

ThomThomson self-requested a review November 14, 2023 22:06

pgayvallet approved these changes Nov 15, 2023

View reviewed changes

ThomThomson approved these changes Nov 15, 2023

View reviewed changes

Heenawter added 3 commits November 15, 2023 12:50

Merge branch 'main' into links-register-alias_2023-10-17

4ddfef2

Merge branch 'main' into links-register-alias_2023-10-17

7ac5512

Merge branch 'main' into links-register-alias_2023-10-17

9558736

vadimkibana approved these changes Nov 20, 2023

View reviewed changes

drewdaemon reviewed Nov 20, 2023

View reviewed changes

src/plugins/visualizations/public/vis_types/vis_type_alias_registry.ts Outdated Show resolved Hide resolved

Heenawter and others added 5 commits November 20, 2023 15:39

Merge branch 'main' into links-register-alias_2023-10-17

e478165

Merge branch 'main' into links-register-alias_2023-10-17

991c416

Decouple embeddable plugin from visualize listing page

f73100a

Make alias optional

be38830

Merge branch 'main' into links-register-alias_2023-10-17

df35a2c

drewdaemon approved these changes Nov 21, 2023

View reviewed changes

Heenawter added 3 commits November 21, 2023 16:15

Fix types

70c98e1

Fix more types

99df0f9

Fix jest test

554f844

ryankeairns approved these changes Nov 22, 2023

View reviewed changes

Heenawter merged commit 8eaebb6 into elastic:main Nov 22, 2023

Heenawter deleted the links-register-alias_2023-10-17 branch November 22, 2023 17:37

kibanamachine added the v8.12.0 label Nov 22, 2023

drewdaemon mentioned this pull request Feb 13, 2024

[Visualize library][Serverless] links to legacy vis editor are still active in library #176742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard Navigation] Add Links to Visualization library #170810

[Dashboard Navigation] Add Links to Visualization library #170810

Heenawter commented Nov 7, 2023 •

edited

Loading

Heenawter Nov 14, 2023

ThomThomson Nov 15, 2023

Heenawter Nov 14, 2023

Heenawter Nov 14, 2023

elasticmachine commented Nov 14, 2023

ThomThomson left a comment

ThomThomson Nov 15, 2023

vadimkibana left a comment

drewdaemon left a comment

drewdaemon left a comment

kibana-ci commented Nov 22, 2023

API count

[Dashboard Navigation] Add Links to Visualization library #170810

[Dashboard Navigation] Add Links to Visualization library #170810

Conversation

Heenawter commented Nov 7, 2023 • edited Loading

Summary

Checklist

For maintainers

Heenawter Nov 14, 2023

Choose a reason for hiding this comment

ThomThomson Nov 15, 2023

Choose a reason for hiding this comment

Heenawter Nov 14, 2023

Choose a reason for hiding this comment

Heenawter Nov 14, 2023

Choose a reason for hiding this comment

elasticmachine commented Nov 14, 2023

ThomThomson left a comment

Choose a reason for hiding this comment

ThomThomson Nov 15, 2023

Choose a reason for hiding this comment

vadimkibana left a comment

Choose a reason for hiding this comment

drewdaemon left a comment

Choose a reason for hiding this comment

drewdaemon left a comment

Choose a reason for hiding this comment

kibana-ci commented Nov 22, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Heenawter commented Nov 7, 2023 •

edited

Loading