[Enterprise Search] Support active nav links that have both subnav & …

…non-subnav child routes (#103036) (#103142) * Update generateNavlink to take an `items` subNav and use it to determine isSelected + change getNavLinkActive to early returns + tweak tests for readability * Update WS nav Sources link - to show active on creation routes but not on single source routes * Update AS nav Engines link - should eventually show active on creation routes but not on single engine routes * Update AS engine creation routing - so that it correctly shows as a child route of the Engines link + update breadcrumbs Co-authored-by: Constance <[email protected]>
elastic · Jun 23, 2021 · 4db18c0 · 4db18c0
1 parent e35d66c
commit 4db18c0
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 28 deletions.
diff --git a/...rise_search/public/applications/app_search/components/engine_creation/engine_creation.tsx b/...rise_search/public/applications/app_search/components/engine_creation/engine_creation.tsx
@@ -22,6 +22,7 @@ import {
   EuiButton,
 } from '@elastic/eui';
 
+import { ENGINES_TITLE } from '../engines';
 import { AppSearchPageTemplate } from '../layout';
 
 import {
@@ -43,7 +44,7 @@ export const EngineCreation: React.FC = () => {
 
   return (
     <AppSearchPageTemplate
-      pageChrome={[ENGINE_CREATION_TITLE]}
+      pageChrome={[ENGINES_TITLE, ENGINE_CREATION_TITLE]}
       pageHeader={{ pageTitle: ENGINE_CREATION_TITLE }}
       data-test-subj="EngineCreation"
     >

diff --git a/..._search/public/applications/app_search/components/engines/components/empty_state.test.tsx b/..._search/public/applications/app_search/components/engines/components/empty_state.test.tsx
@@ -53,7 +53,7 @@ describe('EmptyState', () => {
       });
 
       it('sends a user to engine creation', () => {
-        expect(button.prop('to')).toEqual('/engine_creation');
+        expect(button.prop('to')).toEqual('/engines/new');
       });
     });
   });

diff --git a/...k/plugins/enterprise_search/public/applications/app_search/components/layout/nav.test.tsx b/...k/plugins/enterprise_search/public/applications/app_search/components/layout/nav.test.tsx
@@ -8,7 +8,7 @@
 import { setMockValues } from '../../../__mocks__/kea_logic';
 
 jest.mock('../../../shared/layout', () => ({
-  generateNavLink: jest.fn(({ to }) => ({ href: to })),
+  generateNavLink: jest.fn(({ to, items }) => ({ href: to, items })),
 }));
 jest.mock('../engine/engine_nav', () => ({
   useEngineNav: () => [],

diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/layout/nav.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/layout/nav.tsx
@@ -28,8 +28,12 @@ export const useAppSearchNav = () => {
     {
       id: 'engines',
       name: ENGINES_TITLE,
-      ...generateNavLink({ to: ENGINES_PATH, isRoot: true }),
-      items: useEngineNav(),
+      ...generateNavLink({
+        to: ENGINES_PATH,
+        isRoot: true,
+        shouldShowActiveForSubroutes: true,
+        items: useEngineNav(),
+      }),
     },
   ];
 

diff --git a/...h/public/applications/app_search/components/meta_engine_creation/meta_engine_creation.tsx b/...h/public/applications/app_search/components/meta_engine_creation/meta_engine_creation.tsx
@@ -25,6 +25,7 @@ import {
 } from '@elastic/eui';
 
 import { AppLogic } from '../../app_logic';
+import { ENGINES_TITLE } from '../engines';
 import { AppSearchPageTemplate } from '../layout';
 
 import {
@@ -73,7 +74,7 @@ export const MetaEngineCreation: React.FC = () => {
 
   return (
     <AppSearchPageTemplate
-      pageChrome={[META_ENGINE_CREATION_TITLE]}
+      pageChrome={[ENGINES_TITLE, META_ENGINE_CREATION_TITLE]}
       pageHeader={{
         pageTitle: META_ENGINE_CREATION_TITLE,
         description: (

diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/index.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/index.tsx
@@ -104,9 +104,6 @@ export const AppSearchConfigured: React.FC<Required<InitialAppData>> = (props) =
       <Route exact path={ENGINES_PATH}>
         <EnginesOverview />
       </Route>
-      <Route path={ENGINE_PATH}>
-        <EngineRouter />
-      </Route>
       {canManageEngines && (
         <Route exact path={ENGINE_CREATION_PATH}>
           <EngineCreation />
@@ -117,6 +114,9 @@ export const AppSearchConfigured: React.FC<Required<InitialAppData>> = (props) =
           <MetaEngineCreation />
         </Route>
       )}
+      <Route path={ENGINE_PATH}>
+        <EngineRouter />
+      </Route>
       {canViewSettings && (
         <Route exact path={SETTINGS_PATH}>
           <Settings />

diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/routes.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/routes.ts
@@ -18,7 +18,7 @@ export const CREDENTIALS_PATH = '/credentials';
 export const ROLE_MAPPINGS_PATH = '/role_mappings';
 
 export const ENGINES_PATH = '/engines';
-export const ENGINE_CREATION_PATH = '/engine_creation';
+export const ENGINE_CREATION_PATH = `${ENGINES_PATH}/new`; // This is safe from conflicting with an :engineName path because new is a reserved name
 export const ENGINE_PATH = `${ENGINES_PATH}/:engineName`;
 
 export const ENGINE_ANALYTICS_PATH = `${ENGINE_PATH}/analytics`;
@@ -39,7 +39,7 @@ export const ENGINE_REINDEX_JOB_PATH = `${ENGINE_SCHEMA_PATH}/reindex_job/:reind
 export const ENGINE_CRAWLER_PATH = `${ENGINE_PATH}/crawler`;
 // TODO: Crawler sub-pages
 
-export const META_ENGINE_CREATION_PATH = '/meta_engine_creation';
+export const META_ENGINE_CREATION_PATH = `${ENGINES_PATH}/new_meta_engine`; // This is safe from conflicting with an :engineName path because engine names cannot have underscores
 export const META_ENGINE_SOURCE_ENGINES_PATH = `${ENGINE_PATH}/engines`;
 
 export const ENGINE_RELEVANCE_TUNING_PATH = `${ENGINE_PATH}/relevance_tuning`;

diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/layout/nav_link_helpers.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/layout/nav_link_helpers.test.ts
@@ -19,28 +19,37 @@ import { generateNavLink, getNavLinkActive } from './nav_link_helpers';
 describe('generateNavLink', () => {
   beforeEach(() => {
     jest.clearAllMocks();
-    mockKibanaValues.history.location.pathname = '/current_page';
+    mockKibanaValues.history.location.pathname = '/';
   });
 
-  it('generates React Router props & isSelected (active) state for use within an EuiSideNavItem obj', () => {
+  it('generates React Router props for use within an EuiSideNavItem obj', () => {
     const navItem = generateNavLink({ to: '/test' });
 
-    expect(navItem.href).toEqual('/app/enterprise_search/test');
+    expect(navItem).toEqual({
+      href: '/app/enterprise_search/test',
+      onClick: expect.any(Function),
+      isSelected: false,
+    });
 
     navItem.onClick({} as any);
     expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/test');
-
-    expect(navItem.isSelected).toEqual(false);
   });
 
-  describe('getNavLinkActive', () => {
+  describe('isSelected / getNavLinkActive', () => {
     it('returns true when the current path matches the link path', () => {
       mockKibanaValues.history.location.pathname = '/test';
       const isSelected = getNavLinkActive({ to: '/test' });
 
       expect(isSelected).toEqual(true);
     });
 
+    it('return false when the current path does not match the link path', () => {
+      mockKibanaValues.history.location.pathname = '/hello';
+      const isSelected = getNavLinkActive({ to: '/world' });
+
+      expect(isSelected).toEqual(false);
+    });
+
     describe('isRoot', () => {
       it('returns true if the current path is "/"', () => {
         mockKibanaValues.history.location.pathname = '/';
@@ -58,12 +67,42 @@ describe('generateNavLink', () => {
         expect(isSelected).toEqual(true);
       });
 
-      it('returns false if not', () => {
+      /* NOTE: This logic is primarily used for the following routing scenario:
+       * 1. /item/{itemId} shows a child subnav, e.g. /items/{itemId}/settings
+       *    - BUT when the child subnav is open, the parent `Item` nav link should not show as active - its child nav links should
+       * 2. /item/create_item (example) does *not* show a child subnav
+       *    - BUT the parent `Item` nav link should highlight when on this non-subnav route
+       */
+      it('returns false if subroutes already have their own items subnav (with active state)', () => {
+        mockKibanaValues.history.location.pathname = '/items/123/settings';
+        const isSelected = getNavLinkActive({
+          to: '/items',
+          shouldShowActiveForSubroutes: true,
+          items: [{ id: 'settings', name: 'Settings' }],
+        });
+
+        expect(isSelected).toEqual(false);
+      });
+
+      it('returns false if not a valid subroute', () => {
+        mockKibanaValues.history.location.pathname = '/hello/world';
+        const isSelected = getNavLinkActive({ to: '/world', shouldShowActiveForSubroutes: true });
+
+        expect(isSelected).toEqual(false);
+      });
+
+      it('returns false for subroutes if the flag is not passed', () => {
         mockKibanaValues.history.location.pathname = '/hello/world';
         const isSelected = getNavLinkActive({ to: '/hello' });
 
         expect(isSelected).toEqual(false);
       });
     });
   });
+
+  it('optionally passes items', () => {
+    const navItem = generateNavLink({ to: '/test', items: [] });
+
+    expect(navItem.items).toEqual([]);
+  });
 });
diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/layout/nav_link_helpers.ts b/x-pack/plugins/enterprise_search/public/applications/shared/layout/nav_link_helpers.ts
@@ -5,6 +5,8 @@
  * 2.0.
  */
 
+import { EuiSideNavItemType } from '@elastic/eui';
+
 import { stripTrailingSlash } from '../../../../common/strip_slashes';
 
 import { KibanaLogic } from '../kibana';
@@ -14,27 +16,34 @@ interface Params {
   to: string;
   isRoot?: boolean;
   shouldShowActiveForSubroutes?: boolean;
+  items?: Array<EuiSideNavItemType<unknown>>; // Primarily passed if using `items` to determine isSelected - if not, you can just set `items` outside of this helper
 }
 
-export const generateNavLink = ({ to, ...rest }: Params & ReactRouterProps) => {
+export const generateNavLink = ({ to, items, ...rest }: Params & ReactRouterProps) => {
   return {
     ...generateReactRouterProps({ to, ...rest }),
-    isSelected: getNavLinkActive({ to, ...rest }),
+    isSelected: getNavLinkActive({ to, items, ...rest }),
+    items,
   };
 };
 
 export const getNavLinkActive = ({
   to,
   isRoot = false,
   shouldShowActiveForSubroutes = false,
+  items = [],
 }: Params): boolean => {
   const { pathname } = KibanaLogic.values.history.location;
   const currentPath = stripTrailingSlash(pathname);
 
-  const isActive =
-    currentPath === to ||
-    (shouldShowActiveForSubroutes && currentPath.startsWith(to)) ||
-    (isRoot && currentPath === '');
+  if (currentPath === to) return true;
+
+  if (isRoot && currentPath === '') return true;
+
+  if (shouldShowActiveForSubroutes) {
+    if (items.length) return false; // If a nav link has sub-nav items open, never show it as active
+    if (currentPath.startsWith(to)) return true;
+  }
 
-  return isActive;
+  return false;
 };
diff --git a/...ins/enterprise_search/public/applications/workplace_search/components/layout/nav.test.tsx b/...ins/enterprise_search/public/applications/workplace_search/components/layout/nav.test.tsx
@@ -7,7 +7,7 @@
 
 jest.mock('../../../shared/layout', () => ({
   ...jest.requireActual('../../../shared/layout'),
-  generateNavLink: jest.fn(({ to }) => ({ href: to })),
+  generateNavLink: jest.fn(({ to, items }) => ({ href: to, items })),
 }));
 jest.mock('../../views/content_sources/components/source_sub_nav', () => ({
   useSourceSubNav: () => [],

diff --git a/.../plugins/enterprise_search/public/applications/workplace_search/components/layout/nav.tsx b/.../plugins/enterprise_search/public/applications/workplace_search/components/layout/nav.tsx
@@ -33,8 +33,11 @@ export const useWorkplaceSearchNav = () => {
     {
       id: 'sources',
       name: NAV.SOURCES,
-      ...generateNavLink({ to: SOURCES_PATH }),
-      items: useSourceSubNav(),
+      ...generateNavLink({
+        to: SOURCES_PATH,
+        shouldShowActiveForSubroutes: true,
+        items: useSourceSubNav(),
+      }),
     },
     {
       id: 'groups',
-Original file line number
+Diff line change
@@ Expand Up / @@ -53,7 +53,7 @@ describe('EmptyState', () => { @@
           });
           it('sends a user to engine creation', () => {
-            expect(button.prop('to')).toEqual('/engine_creation');
+            expect(button.prop('to')).toEqual('/engines/new');
           });
         });
       });
@@ Expand Down @@