[navigation]feat: make parent item unclickable and fix duplicate item…

…s in landing page. (#7619) * feat: make parent item unclickable Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Changeset file for PR #7619 created/updated * feat: do not show parent item in landing page Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * temp: save Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: update Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * fix: nav group not reflected when switching to analytics workspace Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code based on comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code based on comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
opensearch-project · Aug 9, 2024 · 389fd28 · 389fd28
1 parent 873c79e
commit 389fd28
Show file tree

Hide file tree

Showing 12 changed files with 279 additions and 96 deletions.
diff --git a/changelogs/fragments/7619.yml b/changelogs/fragments/7619.yml
@@ -0,0 +1,2 @@
+feat:
+- Make parent item unclickable and fix duplicate items in landing page. ([#7619](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7619))
diff --git a/src/core/public/chrome/index.ts b/src/core/public/chrome/index.ts
@@ -60,4 +60,4 @@ export { ChromeNavControl, ChromeNavControls } from './nav_controls';
 export { ChromeDocTitle } from './doc_title';
 export { RightNavigationOrder } from './constants';
 export { ChromeRegistrationNavLink, ChromeNavGroupUpdater, NavGroupItemInMap } from './nav_group';
-export { fulfillRegistrationLinksToChromeNavLinks } from './utils';
+export { fulfillRegistrationLinksToChromeNavLinks, LinkItemType, getSortedNavLinks } from './utils';
diff --git a/src/core/public/chrome/nav_group/nav_group_service.ts b/src/core/public/chrome/nav_group/nav_group_service.ts
@@ -14,11 +14,7 @@ import {
 import { map, switchMap, takeUntil } from 'rxjs/operators';
 import { i18n } from '@osd/i18n';
 import { IUiSettingsClient } from '../../ui_settings';
-import {
-  flattenLinksOrCategories,
-  fulfillRegistrationLinksToChromeNavLinks,
-  getOrderedLinksOrCategories,
-} from '../utils';
+import { fulfillRegistrationLinksToChromeNavLinks, getSortedNavLinks } from '../utils';
 import { ChromeNavLinks } from '../nav_links';
 import { InternalApplicationStart } from '../../application';
 import { NavGroupStatus } from '../../../../core/types';
@@ -117,10 +113,8 @@ export class ChromeNavGroupService {
     navGroup: NavGroupItemInMap,
     allValidNavLinks: Array<Readonly<ChromeNavLink>>
   ) {
-    return flattenLinksOrCategories(
-      getOrderedLinksOrCategories(
-        fulfillRegistrationLinksToChromeNavLinks(navGroup.navLinks, allValidNavLinks)
-      )
+    return getSortedNavLinks(
+      fulfillRegistrationLinksToChromeNavLinks(navGroup.navLinks, allValidNavLinks)
     );
   }
 
@@ -267,14 +261,17 @@ export class ChromeNavGroupService {
       if (appId && navGroupMap) {
         const appIdNavGroupMap = new Map<string, Set<string>>();
         // iterate navGroupMap
-        Object.keys(navGroupMap).forEach((navGroupId) => {
-          navGroupMap[navGroupId].navLinks.forEach((navLink) => {
-            const navLinkId = navLink.id;
-            const navGroupSet = appIdNavGroupMap.get(navLinkId) || new Set();
-            navGroupSet.add(navGroupId);
-            appIdNavGroupMap.set(navLinkId, navGroupSet);
+        Object.keys(navGroupMap)
+          // Nav group of Hidden status should be filtered out when counting navGroups the currentApp belongs to
+          .filter((navGroupId) => navGroupMap[navGroupId].status !== NavGroupStatus.Hidden)
+          .forEach((navGroupId) => {
+            navGroupMap[navGroupId].navLinks.forEach((navLink) => {
+              const navLinkId = navLink.id;
+              const navGroupSet = appIdNavGroupMap.get(navLinkId) || new Set();
+              navGroupSet.add(navGroupId);
+              appIdNavGroupMap.set(navLinkId, navGroupSet);
+            });
           });
-        });
 
         const navGroups = appIdNavGroupMap.get(appId);
         if (navGroups && navGroups.size === 1) {

diff --git a/src/core/public/chrome/ui/header/__snapshots__/collapsible_nav_group_enabled.test.tsx.snap b/src/core/public/chrome/ui/header/__snapshots__/collapsible_nav_group_enabled.test.tsx.snap
diff --git a/src/core/public/chrome/ui/header/collapsible_nav_group_enabled.scss b/src/core/public/chrome/ui/header/collapsible_nav_group_enabled.scss
@@ -8,10 +8,6 @@
     margin-bottom: 0;
     margin-top: 0;
 
-    &::after {
-      display: none;
-    }
-
     .nav-link-item-btn {
       margin-bottom: 0;
 
@@ -21,7 +17,32 @@
     }
   }
 
+  .nav-link-parent-item-button {
+    > span {
+      flex-direction: row-reverse;
+
+      > * {
+        margin-right: $euiSizeS;
+        margin-left: 2px;
+      }
+    }
+  }
+
+  .nav-link-fake-item {
+    margin-top: 0;
+  }
+
+  .nav-link-fake-item-button {
+    display: none;
+  }
+
   .nav-nested-item {
+    margin-bottom: 4px;
+
+    &::after {
+      height: unset;
+    }
+
     .nav-link-item-btn {
       padding-left: 0;
       padding-right: 0;
@@ -31,7 +52,7 @@
   .left-navigation-wrapper {
     display: flex;
     flex-direction: column;
-    border-right: $ouiBorderThin;
+    border-right: $euiBorderThin;
   }
 
   .scrollable-container {

diff --git a/src/core/public/chrome/ui/header/collapsible_nav_group_enabled.test.tsx b/src/core/public/chrome/ui/header/collapsible_nav_group_enabled.test.tsx
@@ -43,7 +43,7 @@ describe('<NavGroups />', () => {
   it('should render correctly', () => {
     const navigateToApp = jest.fn();
     const onNavItemClick = jest.fn();
-    const { container, getByTestId } = render(
+    const { container, getByTestId, queryByTestId } = render(
       <NavGroups
         navLinks={[
           getMockedNavLink({
@@ -88,7 +88,14 @@ describe('<NavGroups />', () => {
     expect(container).toMatchSnapshot();
     expect(container.querySelectorAll('.nav-link-item-btn').length).toEqual(5);
     fireEvent.click(getByTestId('collapsibleNavAppLink-pure'));
-    expect(navigateToApp).toBeCalledWith('pure');
+    expect(navigateToApp).toBeCalledTimes(0);
+    // The accordion is collapsed
+    expect(queryByTestId('collapsibleNavAppLink-subLink')).toBeNull();
+
+    // Expand the accordion
+    fireEvent.click(getByTestId('collapsibleNavAppLink-pure'));
+    fireEvent.click(getByTestId('collapsibleNavAppLink-subLink'));
+    expect(navigateToApp).toBeCalledWith('subLink');
   });
 });
 

diff --git a/src/core/public/chrome/ui/header/collapsible_nav_group_enabled.tsx b/src/core/public/chrome/ui/header/collapsible_nav_group_enabled.tsx
@@ -77,6 +77,8 @@ const titleForSeeAll = i18n.translate('core.ui.primaryNav.seeAllLabel', {
   defaultMessage: 'See all...',
 });
 
+const LEVEL_FOR_ROOT_ITEMS = 1;
+
 export function NavGroups({
   navLinks,
   suffix,
@@ -114,7 +116,11 @@ export function NavGroups({
       'aria-label': link.title,
     };
   };
-  const createSideNavItem = (navLink: LinkItem, className?: string): EuiSideNavItemType<{}> => {
+  const createSideNavItem = (
+    navLink: LinkItem,
+    level: number,
+    className?: string
+  ): EuiSideNavItemType<{}> => {
     if (navLink.itemType === LinkItemType.LINK) {
       if (navLink.link.title === titleForSeeAll) {
         const navItem = createNavItem({
@@ -135,18 +141,43 @@ export function NavGroups({
     }
 
     if (navLink.itemType === LinkItemType.PARENT_LINK && navLink.link) {
-      return {
-        ...createNavItem({ link: navLink.link }),
+      const props = createNavItem({ link: navLink.link });
+      const parentItem = {
+        ...props,
         forceOpen: true,
-        items: navLink.links.map((subNavLink) => createSideNavItem(subNavLink, 'nav-nested-item')),
+        /**
+         * The href and onClick should both be undefined to make parent item rendered as accordion.
+         */
+        href: undefined,
+        onClick: undefined,
+        className: classNames(props.className, 'nav-link-parent-item'),
+        buttonClassName: classNames(props.buttonClassName, 'nav-link-parent-item-button'),
+        items: navLink.links.map((subNavLink) =>
+          createSideNavItem(subNavLink, level + 1, 'nav-nested-item')
+        ),
       };
+      /**
+       * OuiSideBar will never render items of first level as accordion,
+       * in order to display accordion, we need to render a fake parent item.
+       */
+      if (level === LEVEL_FOR_ROOT_ITEMS) {
+        return {
+          className: 'nav-link-fake-item',
+          buttonClassName: 'nav-link-fake-item-button',
+          name: '',
+          items: [parentItem],
+          id: `fake_${props.id}`,
+        };
+      }
+
+      return parentItem;
     }
 
     if (navLink.itemType === LinkItemType.CATEGORY) {
       return {
         id: navLink.category?.id ?? '',
         name: <div className="nav-link-item">{navLink.category?.label ?? ''}</div>,
-        items: navLink.links?.map((link) => createSideNavItem(link)),
+        items: navLink.links?.map((link) => createSideNavItem(link, level + 1)),
         'aria-label': navLink.category?.label,
       };
     }
@@ -155,7 +186,7 @@ export function NavGroups({
   };
   const orderedLinksOrCategories = getOrderedLinksOrCategories(navLinks);
   const sideNavItems = orderedLinksOrCategories
-    .map((navLink) => createSideNavItem(navLink))
+    .map((navLink) => createSideNavItem(navLink, LEVEL_FOR_ROOT_ITEMS))
     .filter((item): item is EuiSideNavItemType<{}> => !!item);
   return (
     <EuiFlexItem style={style}>

diff --git a/src/core/public/chrome/utils.test.ts b/src/core/public/chrome/utils.test.ts
@@ -10,7 +10,7 @@ import {
   fulfillRegistrationLinksToChromeNavLinks,
   getOrderedLinks,
   getOrderedLinksOrCategories,
-  flattenLinksOrCategories,
+  getSortedNavLinks,
 } from './utils';
 
 const mockedNonCategoryLink = {
@@ -47,6 +47,15 @@ const mockedNavLinkB = {
   order: 5,
 };
 
+const mockedSubNavLinkA = {
+  id: 'sub_a',
+  parentNavLinkId: 'a',
+  title: 'sub_a',
+  baseUrl: '',
+  href: '',
+  order: 10,
+};
+
 describe('getAllCategories', () => {
   it('should return all categories', () => {
     const links = {
@@ -124,15 +133,15 @@ describe('getOrderedLinksOrCategories', () => {
   });
 });
 
-describe('flattenLinksOrCategories', () => {
+describe('getSortedNavLinks', () => {
   it('should return flattened links', () => {
-    const navLinks = [mockedNonCategoryLink, mockedNavLinkA, mockedNavLinkB];
-    const orderedLinks = getOrderedLinksOrCategories(navLinks);
-    const flattenedLinks = flattenLinksOrCategories(orderedLinks);
-    expect(flattenedLinks.map((item) => item.id)).toEqual([
+    const navLinks = [mockedNonCategoryLink, mockedNavLinkA, mockedNavLinkB, mockedSubNavLinkA];
+    const sortedNavLinks = getSortedNavLinks(navLinks);
+    expect(sortedNavLinks.map((item) => item.id)).toEqual([
       mockedNavLinkB.id,
       mockedNonCategoryLink.id,
       mockedNavLinkA.id,
+      mockedSubNavLinkA.id,
     ]);
   });
 });