Skip to content

Commit

Permalink
[Enterprise Search] Support active nav links that have both subnav & …
Browse files Browse the repository at this point in the history
…non-subnav child routes (#103036)

* 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
  • Loading branch information
Constance authored Jun 23, 2021
1 parent 3864fe1 commit 045a32b
Show file tree
Hide file tree
Showing 11 changed files with 85 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
EuiButton,
} from '@elastic/eui';

import { ENGINES_TITLE } from '../engines';
import { AppSearchPageTemplate } from '../layout';

import {
Expand All @@ -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"
>
Expand Down
Original file line number Diff 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
Original file line number Diff line number Diff line change
Expand Up @@ -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: () => [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}),
},
];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
} from '@elastic/eui';

import { AppLogic } from '../../app_logic';
import { ENGINES_TITLE } from '../engines';
import { AppSearchPageTemplate } from '../layout';

import {
Expand Down Expand Up @@ -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: (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 />
Expand All @@ -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 />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`;
Expand All @@ -39,7 +39,7 @@ export const ENGINE_REINDEX_JOB_PATH = `${ENGINE_SCHEMA_PATH}/reindex_job/:reind
export const ENGINE_CRAWLER_PATH = `${ENGINE_PATH}/crawler`;
export const ENGINE_CRAWLER_DOMAIN_PATH = `${ENGINE_CRAWLER_PATH}/domains/:domainId`;

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`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '/';
Expand All @@ -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([]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
* 2.0.
*/

import { EuiSideNavItemType } from '@elastic/eui';

import { stripTrailingSlash } from '../../../../common/strip_slashes';

import { KibanaLogic } from '../kibana';
Expand All @@ -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;
};
Original file line number Diff line number Diff line change
Expand Up @@ -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: () => [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down

0 comments on commit 045a32b

Please sign in to comment.