[ChromeNavPackage] Refactor to work with ChromeNavigationNode by sebelga · Pull Request #156526 · elastic/kibana

sebelga · 2023-05-03T12:29:13Z

In this PR I've refactored the @kbn/shared-ux-chrome-navigation to (hopefully) simplify the data model. This is a preparation work to be able to expose an api on the core Chrome service to set the navigation.

Goal

Externally: work with a single interface to define a navigation node: ChromeNavigationNode
Internally: have a single interface for all navigation node definition: ChromeNavigationNodeViewModel
Don't expose component props interface for the navigation configuration.
Only convert the ChromeNavigationNodeViewModel to eui nav props at the component level

What it unblocks

The ChromeNavigationNode and ChromeNavigationNodeViewModel will be imported by the Chrome service
The Chrome service will expose an api to convert a ChromeNavigationNode[] to a ChromeNavigationNodeViewModel[]: converting the link (pointing to appIds or deeplinkIds) to propert href

sebelga · 2023-05-03T12:30:34Z

packages/shared-ux/chrome/navigation/src/model/platform_nav/analytics.ts

+import { ChromeNavigationNodeViewModel } from '../../../types';

-export const analyticsItemSet: NavItemProps[] = [
+// TODO: Declare ChromeNavigationNode[] (with "link" to app id or deeplink id)


The idea is to not use href here but link. The same way links will be defined when calling the chrome.project.setNavigation(/*<config-with-nodes-and-links>*/).

This can be done as a second chunk of work to limit the changes in this PR.

sebelga · 2023-05-03T12:40:40Z

packages/shared-ux/chrome/navigation/src/model/create_side_nav.ts

-    }, []);
-
-  return createSideNavData;
+export const parseNavItems = (


This utility handler is only in charge of building the path of each node and to remove any node that is not enabled.

The conversion to a MyEuiSideNavItem will occur at the component level (inside navigation_bucket.tsx)

packages/shared-ux/chrome/navigation/src/ui/navigation.tsx

sebelga · 2023-05-03T12:44:04Z

packages/shared-ux/chrome/navigation/src/ui/navigation_bucket.tsx

-export const NavigationBucket = (opts: NavigationBucketProps) => {
-  const { id, items, activeNavItemId, ...props } = opts;
-  const { navIsOpen } = useNavigation();
+const navigationNodeToEuiItem = (


This is where we convert our ChromeNavigationNodeViewModel to a EuiSideNavItemType to render the jsx.

Dosant

Looks great!

packages/shared-ux/chrome/navigation/types/index.ts

Dosant · 2023-05-03T13:55:01Z

packages/shared-ux/chrome/navigation/types/index.ts

+   * Full path that points to this node (includes all parent ids). If not set
+   * the path is the id
+   */
+  path?: string;


For what do you think this will be used?
Wouldn't path: string[] be more convenient?
Will this path include this node as the last item? or only parents?

This path includes everything. We could keep an array, but at the same time we can always build the array later if needed.

const path: string = 'stackManagement.someIntermediateSection.indexManagement.someDeepLinkId'

Dosant · 2023-05-03T13:57:52Z

packages/shared-ux/chrome/navigation/types/index.ts

+   */
+  path?: string;
+  isActive?: boolean;
+  href?: string;


I think we should repeat what Chrome uses to generate the current side nav:

href is an absolute path used to enable right-click navigation/open in a new tab

url is a relative path used to call navigate() in onclick handler to prevent page reload

kibana/packages/core/chrome/core-chrome-browser-internal/src/nav_links/to_nav_link.ts

Lines 25 to 26 in ae251e1

const url = appendAppPath(relativeBaseUrl, deepLink?.path || app.defaultPath);

const href = relativeToAbsolute(url);

Basically I agree with an idea, but we will likely need to add more things here, I don't mind doing it as we go

I like the idea. Not sure about naming but we can see later. I wonder if url for absolute and relativePath for navigate purpose?

I don't think href needs to be an absolute path. You can still right-click the link and choose to open it in a new tab when you use a relative string for href. It should be fine to use the same string for the href attribute and navigating. Unless there is some corner case I'm not aware of. But that's how it worked in the POC.

sebelga · 2023-05-03T15:08:09Z

I've tested the PR using @tsullivan PR for search navigation (#156465) and it works well except the active state. This was expected as the active route should be provided by the Chrome service.

You can test it by creating this file inside the serverless_search x-pack plugin

// nav.tsx

/*
 * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
 * or more contributor license agreements. Licensed under the Elastic License
 * 2.0; you may not use this file except in compliance with the Elastic License
 * 2.0.
 */

import { CoreStart } from '@kbn/core/public';
import {
  Navigation,
  NavigationKibanaProvider,
  ChromeNavigationNodeViewModel,
} from '@kbn/shared-ux-chrome-navigation';
import React from 'react';

const navItems: ChromeNavigationNodeViewModel[] = [
  {
    title: '',
    id: 'root',
    items: [
      { id: 'overview', title: 'Overview', href: '/app/enterprise_search/overview' },
      { id: 'indices', title: 'Indices', href: '/app/enterprise_search/content/search_indices' },
      { id: 'engines', title: 'Engines', href: '/app/enterprise_search/content/engines' },
      { id: 'api_keys', title: 'API keys', href: '/app/management/security/api_keys' },
      {
        id: 'ingest_pipelines',
        title: 'Ingest pipelines',
        href: '/app/management/ingest/ingest_pipelines',
      },
    ],
  },
];

export const serverlessSearchSideNavComponentProvider = (core: CoreStart) => () =>
  (
    <NavigationKibanaProvider core={core}>
      <Navigation
        navigationTree={[
          {
            id: 'search_project_nav',
            items: navItems,
            title: 'Search',
            icon: 'logoEnterpriseSearch',
          },
        ]}
        activeNavItemId="search_project_nav.root.overview"
        platformConfig={{}}
        homeHref="/app/enterprise_search/content/setup_guide"
        linkToCloud="projects"
      />
    </NavigationKibanaProvider>
  );

And then update the public/plugin.ts file

// public/plugin.ts

...

public start(
  core: CoreStart,
  _startDeps: ServerlessSearchPluginStartDependencies
): ServerlessSearchPluginStart {
  core.chrome.project.setSideNavComponent(serverlessSearchSideNavComponentProvider(core));
  return {};
}

elasticmachine · 2023-05-03T15:10:15Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

tsullivan

LGTM

sebelga · 2023-05-04T07:44:56Z

@elasticmachine merge upstream

kibana-ci · 2023-05-04T08:59:09Z

💚 Build Succeeded

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
`@kbn/shared-ux-chrome-navigation`	11	15	+4

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/shared-ux-chrome-navigation`	3	2	-1

Unknown metric groups

API count

id	before	after	diff
`@kbn/shared-ux-chrome-navigation`	21	31	+10

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	398	401	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	478	481	+3
total			+5

History

💔 Build #125211 failed 2ed79be
💔 Build #125131 failed d2b2c4e

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

cc @sebelga

…#156526

Refactor shared-ux chrome nav package to work with ChromeNavigationNode

d2b2c4e

sebelga commented May 3, 2023

View reviewed changes

packages/shared-ux/chrome/navigation/src/ui/navigation.tsx Show resolved Hide resolved

sebelga commented May 3, 2023

View reviewed changes

Dosant reviewed May 3, 2023

View reviewed changes

Fix jest test

2ed79be

sebelga marked this pull request as ready for review May 3, 2023 15:09

sebelga requested a review from a team as a code owner May 3, 2023 15:09

sebelga requested a review from tsullivan May 3, 2023 15:09

sebelga self-assigned this May 3, 2023

sebelga added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// Project:Serverless MVP labels May 3, 2023

tsullivan approved these changes May 3, 2023

View reviewed changes

Merge branch 'main' into chrome-nav/set-navigation-api

e895e8f

Dosant approved these changes May 4, 2023

View reviewed changes

sebelga merged commit 5a65112 into elastic:main May 4, 2023

sebelga deleted the chrome-nav/set-navigation-api branch May 4, 2023 09:41

kibanamachine added v8.9.0 backport:skip This PR does not require backporting labels May 4, 2023

sebelga mentioned this pull request May 4, 2023

Highlight current menu item as active #154668

Closed

3 tasks

tsullivan added a commit to tsullivan/kibana that referenced this pull request May 4, 2023

[ChromeNavPackage] Refactor to work with ChromeNavigationNode elastic…

4945a14

…#156526

	const url = appendAppPath(relativeBaseUrl, deepLink?.path \|\| app.defaultPath);
	const href = relativeToAbsolute(url);

Conversation

sebelga commented May 3, 2023

Goal

What it unblocks

Uh oh!

sebelga May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga May 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sebelga May 3, 2023

Choose a reason for hiding this comment

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dosant May 3, 2023

Choose a reason for hiding this comment

Uh oh!

sebelga May 3, 2023

Choose a reason for hiding this comment

Uh oh!

Dosant May 3, 2023

Choose a reason for hiding this comment

Uh oh!

sebelga May 3, 2023

Choose a reason for hiding this comment

Uh oh!

tsullivan May 3, 2023

Choose a reason for hiding this comment

Uh oh!

sebelga commented May 3, 2023

Uh oh!

elasticmachine commented May 3, 2023

Uh oh!

tsullivan left a comment

Choose a reason for hiding this comment

Uh oh!

sebelga commented May 4, 2023

Uh oh!

kibana-ci commented May 4, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sebelga May 3, 2023 •

edited

Loading