From e6e3b16ee1d7a98f2f5b6fcb2f00a62c6a12a301 Mon Sep 17 00:00:00 2001 From: Constance Date: Wed, 7 Apr 2021 14:18:36 -0700 Subject: [PATCH] [Enterprise Search] Change last breadcrumb to inactive/non-linked breadcrumb (#96489) * Update our EUI breadcrumb helper to skip generating links for the last breadcrumb in the list * Fix useEuiBreadcrumbs tests - add a describe block to make it clear we're testing link behavior in non-last breadcrumbs - add a helper that automatically adds a last breadcrumb so that link generation still works * Add comment/note as to why I didn't add last-breadcrumb-specific logic to useGenerateBreadcrumbs --- .../generate_breadcrumbs.test.ts | 75 +++++++++---------- .../kibana_chrome/generate_breadcrumbs.ts | 10 ++- 2 files changed, 42 insertions(+), 43 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.test.ts index 0bf7d618c33b3..c05c4dcbdddc0 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.test.ts @@ -14,6 +14,7 @@ jest.mock('../react_router_helpers', () => ({ import { letBrowserHandleEvent } from '../react_router_helpers'; import { + Breadcrumb, useGenerateBreadcrumbs, useEuiBreadcrumbs, useEnterpriseSearchBreadcrumbs, @@ -40,6 +41,9 @@ describe('useGenerateBreadcrumbs', () => { { text: 'Groups', path: '/groups' }, { text: 'Example Group Name', path: '/groups/{id}' }, { text: 'Source Prioritization', path: '/groups/{id}/source_prioritization' }, + // Note: We're still generating a path for the last breadcrumb even though useEuiBreadcrumbs + // will not render a link for it. This is because it's easier to keep our last-breadcrumb-specific + // logic in one place, & this way we still have a current path if (for some reason) we need it later. ]); }); @@ -89,48 +93,51 @@ describe('useEuiBreadcrumbs', () => { }, { text: 'World', - href: '/app/enterprise_search/world', - onClick: expect.any(Function), + // Per EUI best practices, the last breadcrumb is inactive/is not a link }, ]); }); - it('prevents default navigation and uses React Router history on click', () => { - const breadcrumb = useEuiBreadcrumbs([{ text: '', path: '/test' }])[0] as any; + describe('link behavior for non-last breadcrumbs', () => { + // Test helper - adds a 2nd dummy breadcrumb so that paths from the first breadcrumb are generated + const useEuiBreadcrumb = (breadcrumb: Breadcrumb) => + useEuiBreadcrumbs([breadcrumb, { text: '' }])[0] as any; - expect(breadcrumb.href).toEqual('/app/enterprise_search/test'); - expect(mockHistory.createHref).toHaveBeenCalled(); + it('prevents default navigation and uses React Router history on click', () => { + const breadcrumb = useEuiBreadcrumb({ text: '', path: '/test' }); - const event = { preventDefault: jest.fn() }; - breadcrumb.onClick(event); + expect(breadcrumb.href).toEqual('/app/enterprise_search/test'); + expect(mockHistory.createHref).toHaveBeenCalled(); - expect(event.preventDefault).toHaveBeenCalled(); - expect(mockKibanaValues.navigateToUrl).toHaveBeenCalled(); - }); + const event = { preventDefault: jest.fn() }; + breadcrumb.onClick(event); - it('does not call createHref if shouldNotCreateHref is passed', () => { - const breadcrumb = useEuiBreadcrumbs([ - { text: '', path: '/test', shouldNotCreateHref: true }, - ])[0] as any; + expect(event.preventDefault).toHaveBeenCalled(); + expect(mockKibanaValues.navigateToUrl).toHaveBeenCalled(); + }); - expect(breadcrumb.href).toEqual('/test'); - expect(mockHistory.createHref).not.toHaveBeenCalled(); - }); + it('does not call createHref if shouldNotCreateHref is passed', () => { + const breadcrumb = useEuiBreadcrumb({ text: '', path: '/test', shouldNotCreateHref: true }); - it('does not prevent default browser behavior on new tab/window clicks', () => { - const breadcrumb = useEuiBreadcrumbs([{ text: '', path: '/' }])[0] as any; + expect(breadcrumb.href).toEqual('/test'); + expect(mockHistory.createHref).not.toHaveBeenCalled(); + }); - (letBrowserHandleEvent as jest.Mock).mockImplementationOnce(() => true); - breadcrumb.onClick(); + it('does not prevent default browser behavior on new tab/window clicks', () => { + const breadcrumb = useEuiBreadcrumb({ text: '', path: '/' }); - expect(mockKibanaValues.navigateToUrl).not.toHaveBeenCalled(); - }); + (letBrowserHandleEvent as jest.Mock).mockImplementationOnce(() => true); + breadcrumb.onClick(); + + expect(mockKibanaValues.navigateToUrl).not.toHaveBeenCalled(); + }); - it('does not generate link behavior if path is excluded', () => { - const breadcrumb = useEuiBreadcrumbs([{ text: 'Unclickable breadcrumb' }])[0]; + it('does not generate link behavior if path is excluded', () => { + const breadcrumb = useEuiBreadcrumb({ text: 'Unclickable breadcrumb' }); - expect(breadcrumb.href).toBeUndefined(); - expect(breadcrumb.onClick).toBeUndefined(); + expect(breadcrumb.href).toBeUndefined(); + expect(breadcrumb.onClick).toBeUndefined(); + }); }); }); @@ -164,8 +171,6 @@ describe('useEnterpriseSearchBreadcrumbs', () => { }, { text: 'Page 2', - href: '/app/enterprise_search/page2', - onClick: expect.any(Function), }, ]); }); @@ -174,8 +179,6 @@ describe('useEnterpriseSearchBreadcrumbs', () => { expect(useEnterpriseSearchBreadcrumbs()).toEqual([ { text: 'Enterprise Search', - href: '/app/enterprise_search/overview', - onClick: expect.any(Function), }, ]); }); @@ -219,8 +222,6 @@ describe('useAppSearchBreadcrumbs', () => { }, { text: 'Page 2', - href: '/app/enterprise_search/app_search/page2', - onClick: expect.any(Function), }, ]); }); @@ -234,8 +235,6 @@ describe('useAppSearchBreadcrumbs', () => { }, { text: 'App Search', - href: '/app/enterprise_search/app_search/', - onClick: expect.any(Function), }, ]); }); @@ -279,8 +278,6 @@ describe('useWorkplaceSearchBreadcrumbs', () => { }, { text: 'Page 2', - href: '/app/enterprise_search/workplace_search/page2', - onClick: expect.any(Function), }, ]); }); @@ -294,8 +291,6 @@ describe('useWorkplaceSearchBreadcrumbs', () => { }, { text: 'Workplace Search', - href: '/app/enterprise_search/workplace_search/', - onClick: expect.any(Function), }, ]); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.ts b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.ts index 908cc0601ab9c..5855dc6990f6a 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.ts @@ -24,7 +24,7 @@ import { letBrowserHandleEvent, createHref } from '../react_router_helpers'; * Types */ -interface Breadcrumb { +export interface Breadcrumb { text: string; path?: string; // Used to navigate outside of the React Router basename, @@ -64,16 +64,20 @@ export const useGenerateBreadcrumbs = (trail: BreadcrumbTrail): Breadcrumbs => { /** * Convert IBreadcrumb objects to React-Router-friendly EUI breadcrumb objects * https://elastic.github.io/eui/#/navigation/breadcrumbs + * + * NOTE: Per EUI best practices, we remove the link behavior and + * generate an inactive breadcrumb for the last breadcrumb in the list. */ export const useEuiBreadcrumbs = (breadcrumbs: Breadcrumbs): EuiBreadcrumb[] => { const { navigateToUrl, history } = useValues(KibanaLogic); const { http } = useValues(HttpLogic); - return breadcrumbs.map(({ text, path, shouldNotCreateHref }) => { + return breadcrumbs.map(({ text, path, shouldNotCreateHref }, i) => { const breadcrumb: EuiBreadcrumb = { text }; + const isLastBreadcrumb = i === breadcrumbs.length - 1; - if (path) { + if (path && !isLastBreadcrumb) { breadcrumb.href = createHref(path, { history, http }, { shouldNotCreateHref }); breadcrumb.onClick = (event) => { if (letBrowserHandleEvent(event)) return;