[Navigation embeddable] Add content management #160896

nickpeihl · 2023-06-29T14:17:33Z

Summary

Adds content management to navigation embeddable feature branch.

Allows Links panels to be by-value or by-reference on a Dashboard. The UX for users to choose to save by-value or by-reference remains to be finalized and is out of scope for this PR.

…fix'

…bana into navEmbeddable-cm

…into navEmbeddable-cm

…-ref HEAD~1..HEAD --fix'

…into navEmbeddable-cm

…vEmbeddable-cm

…fix'

…te --fix'

Heenawter

Initial review - left a few questions + comments, and I caught a couple bugs that you may already know about. This is coming along great! 🎉

src/plugins/navigation_embeddable/public/embeddable/types.ts

src/plugins/navigation_embeddable/public/components/navigation_embeddable_component.tsx

src/plugins/navigation_embeddable/public/services/attribute_service.ts

src/plugins/navigation_embeddable/public/embeddable/navigation_embeddable.tsx

src/plugins/navigation_embeddable/public/components/navigation_embeddable_panel_editor.tsx

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

This makes it easier to use the Elasticsearch `_update` API for saved objects. With arrays we are always replacing the entire selection. Using `_update` with an object mapping applies only partial updates. So it was not possible to delete a link.

The `createNewEmbeddable` function will catch the rejected promise and return to the dashboard.

nickpeihl · 2023-08-07T17:34:28Z

src/plugins/embeddable/public/lib/actions/edit_panel_action.ts

+        embeddable.inputIsRefType(embeddable.getInput())
+      ) {
+        embeddable.reload();
+      }


We can make this better by making changes to the Container.replaceEmbeddable method to be functionally similar to the replacePanel function in dashboard/public/dashboard_container/embeddable/api/panel_management.ts.

PR to address this: nickpeihl#2 @nickpeihl @ThomThomson

Figured it was easiest to make a PR against this PR, so that we can test the saved object stuff in its entirety - let me know if something else works better, though 👍

nickpeihl · 2023-08-07T17:35:17Z

src/plugins/navigation_embeddable/common/types.ts

+
+export type NavigationEmbeddableContentType = 'navigation_embeddable';
+
+// TODO does this type need to be versioned?


nickpeihl · 2023-08-07T17:46:15Z

src/plugins/navigation_embeddable/public/components/navigation_embeddable_panel_editor.tsx

+                </EuiFlexItem>
+              ) : null}
+            </EuiFlexGroup>
+          </EuiFlexItem>


We could make this EuiFlyoutFooter into separate components. But I think it is out-of-scope for this PR until we have user testing to answer the question "Is it better to use two buttons in the footer for by-value / by-value creation, or do we want to use the originally proposed EuiSwitch"?

src/plugins/navigation_embeddable/public/components/navigation_embeddable_strings.ts

nickpeihl · 2023-08-07T17:52:06Z

src/plugins/navigation_embeddable/public/embeddable/navigation_embeddable.tsx

+    const { metaInfo, attributes } = await this.attributeService.unwrapAttributes(input);
+    if (this.isDestroyed) return;
+
+    // TODO handle metaInfo


src/plugins/navigation_embeddable/public/embeddable/navigation_embeddable_factory.ts

nickpeihl · 2023-08-07T17:55:28Z

src/plugins/navigation_embeddable/public/embeddable/navigation_embeddable_factory.ts

-    ).catch(() => {
-      // swallow the promise rejection that happens when the flyout is closed
-      return {};
-    });


No need to catch here as the Embeddable panel will handle the catch and return without creating an embeddable.

@nickpeihl I think we still need this catch because we're getting an Unhandled promise rejection error thrown to the console on cancel when editing an existing embeddable:

Screen.Recording.2023-08-07.at.3.38.46.PM.mov

Oops, I still need to look into this. I was under the impression that we should not have to return anything since the createNewEmbeddable method has a catch.

EDIT: Oh, you said "when editing an existing embeddable". 😅

src/plugins/navigation_embeddable/public/services/attribute_service.ts

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

nickpeihl · 2023-08-07T18:04:42Z

Marking this as "Ready for review". I added some 👀 comments that I'm still looking over to see if they are in scope for this PR.

Heenawter

Making good progress! 🎉

src/plugins/navigation_embeddable/public/components/navigation_embeddable_panel_editor.tsx

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

src/plugins/navigation_embeddable/public/components/navigation_embeddable_panel_editor.tsx

src/plugins/navigation_embeddable/public/components/navigation_embeddable_strings.ts

src/plugins/navigation_embeddable/public/editor/navigation_embeddable_editor_tools.tsx

src/plugins/navigation_embeddable/public/components/navigation_embeddable_panel_editor.tsx

These equality of these constants are compared in the replaceEmbeddable function which errors because they were different. It seems this happens in other plugins, too, but we don't see errors since the constants are identical. See `MAP_SAVED_OBJECT_TYPE` and `CONTENT_ID` in the maps plugin. 🙈

…8-07 Replace embeddable + panel changes

…able-cm

kibana-ci · 2023-08-10T15:52:12Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #65 / Console App CCS Perform CCS Search in Console it should be able to access remote data
[job] [logs] FTR Configs #65 / Console App CCS Perform CCS Search in Console it should be able to access remote data
[job] [logs] FTR Configs #28 / Controls Range Slider Control create and edit edits title and size of an existing control and retains existing range selection
[job] [logs] FTR Configs #28 / Controls Range Slider Control create and edit edits title and size of an existing control and retains existing range selection
[job] [logs] FTR Configs #13 / dashboard app - group 5 embed mode default URL params renders as expected
[job] [logs] FTR Configs #13 / dashboard app - group 5 embed mode default URL params renders as expected
[job] [logs] FTR Configs #6 / Fleet Endpoints fleet policy secrets Should correctly create the policy with secrets
[job] [logs] FTR Configs #6 / Fleet Endpoints fleet policy secrets Should correctly create the policy with secrets
[job] [logs] FTR Configs #3 / machine learning - data frame analytics outlier detection creation iowa house prices runs the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #3 / machine learning - data frame analytics outlier detection creation iowa house prices runs the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #59 / MetricsUI Endpoints log highlight apis /log_entries/highlights with the default source highlights built-in message column
[job] [logs] FTR Configs #59 / MetricsUI Endpoints log highlight apis /log_entries/highlights with the default source highlights built-in message column
[job] [logs] FTR Configs #14 / Options list control Dashboard options list creation and editing Options List Control creation and editing experience renames an existing control and retains selection
[job] [logs] FTR Configs #14 / Options list control Dashboard options list creation and editing Options List Control creation and editing experience renames an existing control and retains selection
[job] [logs] FTR Configs #8 / security APIs - Kerberos Kerberos authentication API access with missing access token document or expired refresh token. "before each" hook for "non-AJAX call should initiate SPNEGO and clear existing cookie"
[job] [logs] FTR Configs #20 / security APIs - Kerberos Kerberos authentication API access with missing access token document or expired refresh token. "before each" hook for "non-AJAX call should initiate SPNEGO and clear existing cookie"
[job] [logs] FTR Configs #8 / security APIs - Kerberos Kerberos authentication API access with missing access token document or expired refresh token. "before each" hook for "non-AJAX call should initiate SPNEGO and clear existing cookie"
[job] [logs] FTR Configs #20 / security APIs - Kerberos Kerberos authentication API access with missing access token document or expired refresh token. "before each" hook for "non-AJAX call should initiate SPNEGO and clear existing cookie"
[job] [logs] FTR Configs #51 / security APIs - SAML SAML authentication API access with missing access token document. "before each" hook for "should properly set cookie and redirect user to IdP"
[job] [logs] FTR Configs #51 / security APIs - SAML SAML authentication API access with missing access token document. "before each" hook for "should properly set cookie and redirect user to IdP"
[job] [logs] Jest Integration Tests #2 / SO type registrations does not remove types from registrations without updating excludeOnUpgradeQuery
[job] [logs] FTR Configs #22 / Synthetics API Tests PrivateLocationAddMonitor added an integration for previously added monitor
[job] [logs] FTR Configs #22 / Synthetics API Tests PrivateLocationAddMonitor added an integration for previously added monitor
[job] [logs] FTR Configs #61 / visualize app visual builder tsdb check should show an error when using an unsupported tsdb field type
[job] [logs] FTR Configs #61 / visualize app visual builder tsdb check should show an error when using an unsupported tsdb field type

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [c7485e6]

History

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

Heenawter

This is working flawlessly now - wow! 🎉 Left one tiny nit, and it looks like the promise rejection is still being thrown when cancelling out of the edit action as shown here. But once that's resolved, I think it's ready to be approved 🤩

Heenawter · 2023-08-10T17:08:51Z

src/plugins/embeddable/public/lib/actions/edit_panel_action.ts

@@ -94,6 +94,7 @@ export class EditPanelAction implements Action<ActionContext> {
      const oldExplicitInput = embeddable.getExplicitInput();
      const newExplicitInput = await factory.getExplicitInput(oldExplicitInput, embeddable.parent);
      embeddable.parent?.replaceEmbeddable(embeddable.id, newExplicitInput);
+


nit: Oops - this extra line here is probably from my PR 😅 Probably best to remove it hehe

nickpeihl · 2023-08-10T17:39:21Z

This is working flawlessly now - wow! 🎉 Left one tiny nit, and it looks like the promise rejection is still being thrown when cancelling out of the edit action as shown here. But once that's resolved, I think it's ready to be approved 🤩

Fixed in ce38804

Regarding the CI failures, I think we can merge without fixing most of those since this is a feature branch. We will need to fix failures before merging the feature branch into main. WDYT @Heenawter?

Heenawter · 2023-08-10T17:40:35Z

I think we can merge without fixing most of those since this is a feature branch. We will need to fix failures before merging the feature branch into main.

@nickpeihl Agreed 👍

@ThomThomson

… error handling (#162285) Closes #154357 Closes #161563 ## Summary > **Warning** > I will be waiting to merge this PR until **after** #160896 is merged - I am simply opening it early so that we can start the design review process 👍 ### Layout This PR improves the rendering of the navigation embeddable to include both a horizontal and vertical layout option, as well as changing the style of how the links are rendered: https://github.com/elastic/kibana/assets/8698078/37d27683-a6c4-4e7a-9589-0eb0fb899e98 A known issue with the horizontal layout is that, as demonstrated in the above video, a "compact" horizontal navigation panel does not render as nicely in edit mode versus view mode - this is an **overall panel problem** and not specifically a problem with the navigation embeddable (although the navigation embeddable definitely makes it more obvious). This will be resolved for **all panels** by [removing the panel header altogether](#162182). ### Error handling This PR adds proper error handling to the navigation embeddable so that, if a dashboard link is "broken" (i.e. the destination dashboard has been deleted or cannot be fetched), an appropriate error message shows up in both the component and the editor flyout: https://github.com/elastic/kibana/assets/8698078/33a3e573-36a2-47ca-b367-3e04f9541ca3 > **Note** > When possible, we want to provide the user with as much context as possible for broken dashboard links - that is why, if a dashboard link was given a custom label, we still show this custom label even when the destination dashboard has been deleted/is unreachable. > > However, once a dashboard has been deleted, we no longer know what the title of that dashboard was because the saved object no longer exists - so, if a dashboard link is **not** given a custom label and the destination dashboard is deleted, we default to the "Error fetching dashboard" error message instead. In order to create a distinction between these two scenarios (a broken dashboard link with a custom label versus without), we italicize the generic "Error fetching dashboard" error text. ### Improved efficiency Previously, the navigation embeddable was handling its **own** dashboard cache, which meant that (a) every single embeddable had its own cache and (b) the navigation embeddable code had to be mindful when choosing to use the memoized/cached version of the dashboard versus fetching it fresh. After discussing with @ThomThomson about how to better handle this, we opted to move this logic to the dashboard content management service - not only does this clean up the navigation embeddable code, it also improves all the loading of dashboards in general. For example, consider the following video where I was testing re-loading a previously loaded dashboard on a throttled `Slow 3G` network: https://github.com/elastic/kibana/assets/8698078/41d68ac7-557c-4586-a59b-7268086991dd Notice in the above video how much faster the secondary load of the dashboard is in comparison to the first initial load - this is because, in the second load, we can hit the cache instead of re-fetching the dashboard from the content management client, which allows us to skip an entire loading state. ### 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) - [ ] ~[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~ Will be addressed in #161287 - [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) --------- Co-authored-by: Andrea Del Rio <[email protected]> Co-authored-by: kibanamachine <[email protected]>

nickpeihl and others added 12 commits June 29, 2023 10:13

Initialize content management

0c7c839

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

b426814

…fix'

Merge branch 'navigation-embeddable' of https://github.com/elastic/ki…

38340ab

…bana into navEmbeddable-cm

Rename linkListJSON to linksJSON

78b0bf2

Merge branch 'navEmbeddable-cm' of https://github.com/nickpeihl/kibana …

9e8e10f

…into navEmbeddable-cm

Start wiring content management to UI

81b3bb8

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

8b5ec43

…-ref HEAD~1..HEAD --fix'

Wire saved objects and attribute service

719c046

Merge branch 'navEmbeddable-cm' of https://github.com/nickpeihl/kibana …

82da138

…into navEmbeddable-cm

Merge remote-tracking branch 'upstream/navigation-embeddable' into na…

88df8f2

…vEmbeddable-cm

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

cef9efb

…fix'

[CI] Auto-commit changed files from 'node scripts/check_mappings_upda…

9879a34

…te --fix'

Heenawter mentioned this pull request Jul 25, 2023

[Dashboard Navigation] Add horizontal/vertical embeddable rendering + error handling #162285

Merged

7 tasks

Wires content management to UI. Mostly complete.

6046fde

Heenawter reviewed Jul 25, 2023

View reviewed changes

nickpeihl added 4 commits July 28, 2023 13:41

Link panels must have a minimum of one link

e4d8327

Do not return anything from canceled flyout

f0bd9a9

The `createNewEmbeddable` function will catch the rejected promise and return to the dashboard.

Create by-value or by-ref embeddable

78dd9ce

nickpeihl commented Aug 7, 2023

View reviewed changes

src/plugins/navigation_embeddable/public/components/navigation_embeddable_strings.ts Show resolved Hide resolved

nickpeihl commented Aug 7, 2023

View reviewed changes

src/plugins/navigation_embeddable/public/embeddable/navigation_embeddable_factory.ts Outdated Show resolved Hide resolved

nickpeihl commented Aug 7, 2023

View reviewed changes

src/plugins/navigation_embeddable/public/services/attribute_service.ts Show resolved Hide resolved

nickpeihl commented Aug 7, 2023

View reviewed changes

src/plugins/navigation_embeddable/public/editor/open_editor_flyout.tsx Outdated Show resolved Hide resolved

nickpeihl marked this pull request as ready for review August 7, 2023 18:03

nickpeihl requested a review from a team as a code owner August 7, 2023 18:03

nickpeihl requested a review from Heenawter August 7, 2023 18:04

Fix test failures

c4d0dbc

botelastic bot added the Feature:Embedding Embedding content via iFrame label Aug 7, 2023

Heenawter added 2 commits August 7, 2023 14:31

Replace embeddable + panel changes

e352936

Clean up

2b463fc

Heenawter reviewed Aug 8, 2023

View reviewed changes

nickpeihl added 4 commits August 8, 2023 16:38

Merge pull request #2 from Heenawter/suggested-replace-changes_2023-0…

8fb0ed5

…8-07 Replace embeddable + panel changes

Review feedback

2729cfc

Merge remote-tracking branch 'origin/navEmbeddable-cm' into navEmbedd…

6b254f7

…able-cm

Heenawter reviewed Aug 10, 2023

View reviewed changes

Catch promise rejection when canceling edit panel

ce38804

Heenawter approved these changes Aug 10, 2023

View reviewed changes

nickpeihl merged commit 577134c into elastic:navigation-embeddable Aug 10, 2023

Heenawter mentioned this pull request Aug 14, 2023

[Dashboard Navigation] Save by Reference #154362

Closed

nickpeihl mentioned this pull request Aug 21, 2023

[Dashboard navigation] By-reference Links panels add input attributes to Dashboard saved objects #164322

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Navigation embeddable] Add content management #160896

[Navigation embeddable] Add content management #160896

nickpeihl commented Jun 29, 2023 •

edited

Loading

Heenawter left a comment

nickpeihl Aug 7, 2023

Heenawter Aug 7, 2023 •

edited

Loading

nickpeihl Aug 7, 2023 •

edited

Loading

nickpeihl Aug 7, 2023

nickpeihl Aug 7, 2023

nickpeihl Aug 7, 2023

Heenawter Aug 7, 2023 •

edited

Loading

nickpeihl Aug 10, 2023 •

edited

Loading

nickpeihl commented Aug 7, 2023

Heenawter left a comment

kibana-ci commented Aug 10, 2023 •

edited

Loading

Heenawter left a comment •

edited

Loading

Heenawter Aug 10, 2023

nickpeihl commented Aug 10, 2023

Heenawter commented Aug 10, 2023


		export type NavigationEmbeddableContentType = 'navigation_embeddable';

		// TODO does this type need to be versioned?

[Navigation embeddable] Add content management #160896

[Navigation embeddable] Add content management #160896

Conversation

nickpeihl commented Jun 29, 2023 • edited Loading

Summary

Heenawter left a comment

Choose a reason for hiding this comment

nickpeihl Aug 7, 2023

Choose a reason for hiding this comment

Heenawter Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

nickpeihl Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

nickpeihl Aug 7, 2023

Choose a reason for hiding this comment

nickpeihl Aug 7, 2023

Choose a reason for hiding this comment

nickpeihl Aug 7, 2023

Choose a reason for hiding this comment

Heenawter Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

nickpeihl Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

nickpeihl commented Aug 7, 2023

Heenawter left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 10, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

History

Heenawter left a comment • edited Loading

Choose a reason for hiding this comment

Heenawter Aug 10, 2023

Choose a reason for hiding this comment

nickpeihl commented Aug 10, 2023

Heenawter commented Aug 10, 2023

nickpeihl commented Jun 29, 2023 •

edited

Loading

Heenawter Aug 7, 2023 •

edited

Loading

nickpeihl Aug 7, 2023 •

edited

Loading

Heenawter Aug 7, 2023 •

edited

Loading

nickpeihl Aug 10, 2023 •

edited

Loading

kibana-ci commented Aug 10, 2023 •

edited

Loading

Heenawter left a comment •

edited

Loading