From a64ed95b84c80d1a1b9dde2aa8477ddf9135f744 Mon Sep 17 00:00:00 2001 From: Jason Rhodes Date: Wed, 6 Feb 2019 12:04:12 -0500 Subject: [PATCH] [APM] Updates head title and fixes breadcrumb string issues (#29960) * Solves several breadcrumb string/object problems, adds title to page * Updates breadcrumb tests and adds title tests * Removes switch logic from route config in favor of a single switch around all routes * Adding draft version of withBreadcrumbs HOC with TS errors * ProvideBreadcrumbs implemented * Adds tests to provide breadcrumb logic * Fixed up generics and tests --- x-pack/package.json | 1 - .../app/Main/ProvideBreadcrumbs.tsx | 134 ++++++++++++++++++ ...teBreadcrumbs.ts => UpdateBreadcrumbs.tsx} | 40 +++--- .../Main/__test__/ProvideBreadcrumbs.test.tsx | 100 +++++++++++++ .../Main/__test__/UpdateBreadcrumbs.test.js | 21 ++- .../UpdateBreadcrumbs.test.js.snap | 113 ++------------- .../apm/public/components/app/Main/index.js | 14 +- .../components/app/Main/routeConfig.tsx | 107 ++++++-------- yarn.lock | 5 - 9 files changed, 334 insertions(+), 201 deletions(-) create mode 100644 x-pack/plugins/apm/public/components/app/Main/ProvideBreadcrumbs.tsx rename x-pack/plugins/apm/public/components/app/Main/{UpdateBreadcrumbs.ts => UpdateBreadcrumbs.tsx} (61%) create mode 100644 x-pack/plugins/apm/public/components/app/Main/__test__/ProvideBreadcrumbs.test.tsx diff --git a/x-pack/package.json b/x-pack/package.json index b754153f9813..a0b6d8c6c229 100644 --- a/x-pack/package.json +++ b/x-pack/package.json @@ -233,7 +233,6 @@ "react-portal": "^3.2.0", "react-redux": "^5.0.7", "react-redux-request": "^1.5.6", - "react-router-breadcrumbs-hoc": "1.1.2", "react-router-dom": "^4.3.1", "react-select": "^1.2.1", "react-shortcuts": "^2.0.0", diff --git a/x-pack/plugins/apm/public/components/app/Main/ProvideBreadcrumbs.tsx b/x-pack/plugins/apm/public/components/app/Main/ProvideBreadcrumbs.tsx new file mode 100644 index 000000000000..1c09ab6fb7e4 --- /dev/null +++ b/x-pack/plugins/apm/public/components/app/Main/ProvideBreadcrumbs.tsx @@ -0,0 +1,134 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { Location } from 'history'; +import React from 'react'; +import { + matchPath, + RouteComponentProps, + RouteProps, + withRouter +} from 'react-router-dom'; +import { StringMap } from 'x-pack/plugins/apm/typings/common'; + +type LocationMatch = Pick< + RouteComponentProps>, + 'location' | 'match' +>; + +export type BreadcrumbFunction = (props: LocationMatch) => string; + +export interface BreadcrumbRoute extends RouteProps { + breadcrumb: string | BreadcrumbFunction | null; +} + +export interface Breadcrumb extends LocationMatch { + value: string; +} + +export interface RenderProps extends RouteComponentProps { + breadcrumbs: Breadcrumb[]; +} + +export interface ProvideBreadcrumbsProps extends RouteComponentProps { + routes: BreadcrumbRoute[]; + render: (props: RenderProps) => React.ReactElement | null; +} + +interface ParseOptions extends LocationMatch { + breadcrumb: string | BreadcrumbFunction; +} + +const parse = (options: ParseOptions) => { + const { breadcrumb, match, location } = options; + let value; + + if (typeof breadcrumb === 'function') { + value = breadcrumb({ match, location }); + } else { + value = breadcrumb; + } + + return { value, match, location }; +}; + +export function getBreadcrumb({ + location, + currentPath, + routes +}: { + location: Location; + currentPath: string; + routes: BreadcrumbRoute[]; +}) { + return routes.reduce((found, { breadcrumb, ...route }) => { + if (found) { + return found; + } + + if (!breadcrumb) { + return null; + } + + const match = matchPath>(currentPath, route); + + if (match) { + return parse({ + breadcrumb, + match, + location + }); + } + + return null; + }, null); +} + +export function getBreadcrumbs({ + routes, + location +}: { + routes: BreadcrumbRoute[]; + location: Location; +}) { + const breadcrumbs: Breadcrumb[] = []; + const { pathname } = location; + + pathname + .split('?')[0] + .replace(/\/$/, '') + .split('/') + .reduce((acc, next) => { + // `/1/2/3` results in match checks for `/1`, `/1/2`, `/1/2/3`. + const currentPath = !next ? '/' : `${acc}/${next}`; + const breadcrumb = getBreadcrumb({ + location, + currentPath, + routes + }); + + if (breadcrumb) { + breadcrumbs.push(breadcrumb); + } + + return currentPath === '/' ? '' : currentPath; + }, ''); + + return breadcrumbs; +} + +function ProvideBreadcrumbsComponent({ + routes = [], + render, + location, + match, + history +}: ProvideBreadcrumbsProps) { + const breadcrumbs = getBreadcrumbs({ routes, location }); + return render({ breadcrumbs, location, match, history }); +} + +export const ProvideBreadcrumbs = withRouter(ProvideBreadcrumbsComponent); diff --git a/x-pack/plugins/apm/public/components/app/Main/UpdateBreadcrumbs.ts b/x-pack/plugins/apm/public/components/app/Main/UpdateBreadcrumbs.tsx similarity index 61% rename from x-pack/plugins/apm/public/components/app/Main/UpdateBreadcrumbs.ts rename to x-pack/plugins/apm/public/components/app/Main/UpdateBreadcrumbs.tsx index 45fe37e9bf8c..0bd8a76b8579 100644 --- a/x-pack/plugins/apm/public/components/app/Main/UpdateBreadcrumbs.ts +++ b/x-pack/plugins/apm/public/components/app/Main/UpdateBreadcrumbs.tsx @@ -5,32 +5,28 @@ */ import { Location } from 'history'; -import { flatten } from 'lodash'; +import { last } from 'lodash'; import React from 'react'; -// @ts-ignore -import { withBreadcrumbs } from 'react-router-breadcrumbs-hoc'; import chrome from 'ui/chrome'; import { toQuery } from '../../shared/Links/url_helpers'; +import { Breadcrumb, ProvideBreadcrumbs } from './ProvideBreadcrumbs'; import { routes } from './routeConfig'; interface Props { location: Location; - breadcrumbs: Array<{ - breadcrumb: any; - match: { - url: string; - }; - }>; + breadcrumbs: Breadcrumb[]; } class UpdateBreadcrumbsComponent extends React.Component { public updateHeaderBreadcrumbs() { const { _g = '', kuery = '' } = toQuery(this.props.location.search); - const breadcrumbs = this.props.breadcrumbs.map(({ breadcrumb, match }) => ({ - text: breadcrumb, + const breadcrumbs = this.props.breadcrumbs.map(({ value, match }) => ({ + text: value, href: `#${match.url}?_g=${_g}&kuery=${kuery}` })); + const current = last(breadcrumbs) || { text: '' }; + document.title = current.text; chrome.breadcrumbs.set(breadcrumbs); } @@ -47,12 +43,16 @@ class UpdateBreadcrumbsComponent extends React.Component { } } -const flatRoutes = flatten( - routes.map(route => (route.switchRoutes ? route.switchRoutes : route)) -); - -const UpdateBreadcrumbs = withBreadcrumbs(flatRoutes)( - UpdateBreadcrumbsComponent -); - -export { UpdateBreadcrumbs }; +export function UpdateBreadcrumbs() { + return ( + ( + + )} + /> + ); +} diff --git a/x-pack/plugins/apm/public/components/app/Main/__test__/ProvideBreadcrumbs.test.tsx b/x-pack/plugins/apm/public/components/app/Main/__test__/ProvideBreadcrumbs.test.tsx new file mode 100644 index 000000000000..7826bf16350d --- /dev/null +++ b/x-pack/plugins/apm/public/components/app/Main/__test__/ProvideBreadcrumbs.test.tsx @@ -0,0 +1,100 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { Location } from 'history'; +import { BreadcrumbRoute, getBreadcrumbs } from '../ProvideBreadcrumbs'; + +describe('getBreadcrumbs', () => { + const getTestRoutes = (): BreadcrumbRoute[] => [ + { path: '/a', exact: true, breadcrumb: 'A' }, + { path: '/a/ignored', exact: true, breadcrumb: 'Ignored Route' }, + { + path: '/a/:letter', + exact: true, + breadcrumb: ({ match }) => `Second level: ${match.params.letter}` + }, + { + path: '/a/:letter/c', + exact: true, + breadcrumb: ({ match }) => `Third level: ${match.params.letter}` + } + ]; + + const getLocation = () => + ({ + pathname: '/a/b/c/' + } as Location); + + it('should return a set of matching breadcrumbs for a given path', () => { + const breadcrumbs = getBreadcrumbs({ + location: getLocation(), + routes: getTestRoutes() + }); + + expect(breadcrumbs.map(b => b.value)).toMatchInlineSnapshot(` +Array [ + "A", + "Second level: b", + "Third level: b", +] +`); + }); + + it('should skip breadcrumbs if breadcrumb is null', () => { + const location = getLocation(); + const routes = getTestRoutes(); + + routes[2].breadcrumb = null; + + const breadcrumbs = getBreadcrumbs({ + location, + routes + }); + + expect(breadcrumbs.map(b => b.value)).toMatchInlineSnapshot(` +Array [ + "A", + "Third level: b", +] +`); + }); + + it('should skip breadcrumbs if breadcrumb key is missing', () => { + const location = getLocation(); + const routes = getTestRoutes(); + + delete routes[2].breadcrumb; + + const breadcrumbs = getBreadcrumbs({ location, routes }); + + expect(breadcrumbs.map(b => b.value)).toMatchInlineSnapshot(` +Array [ + "A", + "Third level: b", +] +`); + }); + + it('should produce matching breadcrumbs even if the pathname has a query string appended', () => { + const location = getLocation(); + const routes = getTestRoutes(); + + location.pathname += '?some=thing'; + + const breadcrumbs = getBreadcrumbs({ + location, + routes + }); + + expect(breadcrumbs.map(b => b.value)).toMatchInlineSnapshot(` +Array [ + "A", + "Second level: b", + "Third level: b", +] +`); + }); +}); diff --git a/x-pack/plugins/apm/public/components/app/Main/__test__/UpdateBreadcrumbs.test.js b/x-pack/plugins/apm/public/components/app/Main/__test__/UpdateBreadcrumbs.test.js index 28337f771255..96c8b2d758ac 100644 --- a/x-pack/plugins/apm/public/components/app/Main/__test__/UpdateBreadcrumbs.test.js +++ b/x-pack/plugins/apm/public/components/app/Main/__test__/UpdateBreadcrumbs.test.js @@ -46,37 +46,56 @@ function expectBreadcrumbToMatchSnapshot(route) { } describe('Breadcrumbs', () => { + let realDoc; + beforeEach(() => { + realDoc = global.document; + global.document = { + title: 'Kibana' + }; chrome.breadcrumbs.set.mockReset(); }); + afterEach(() => { + global.document = realDoc; + }); + it('Homepage', () => { expectBreadcrumbToMatchSnapshot('/'); + expect(global.document.title).toMatchInlineSnapshot(`"APM"`); }); it('/:serviceName/errors/:groupId', () => { expectBreadcrumbToMatchSnapshot('/opbeans-node/errors/myGroupId'); + expect(global.document.title).toMatchInlineSnapshot(`"myGroupId"`); }); it('/:serviceName/errors', () => { expectBreadcrumbToMatchSnapshot('/opbeans-node/errors'); + expect(global.document.title).toMatchInlineSnapshot(`"Errors"`); }); it('/:serviceName', () => { expectBreadcrumbToMatchSnapshot('/opbeans-node'); + expect(global.document.title).toMatchInlineSnapshot(`"opbeans-node"`); }); it('/:serviceName/transactions', () => { expectBreadcrumbToMatchSnapshot('/opbeans-node/transactions'); + expect(global.document.title).toMatchInlineSnapshot(`"Transactions"`); }); it('/:serviceName/transactions/:transactionType', () => { expectBreadcrumbToMatchSnapshot('/opbeans-node/transactions/request'); + expect(global.document.title).toMatchInlineSnapshot(`"Transactions"`); }); it('/:serviceName/transactions/:transactionType/:transactionName', () => { expectBreadcrumbToMatchSnapshot( - '/:serviceName/transactions/request/my-transaction-name' + '/opbeans-node/transactions/request/my-transaction-name' + ); + expect(global.document.title).toMatchInlineSnapshot( + `"my-transaction-name"` ); }); }); diff --git a/x-pack/plugins/apm/public/components/app/Main/__test__/__snapshots__/UpdateBreadcrumbs.test.js.snap b/x-pack/plugins/apm/public/components/app/Main/__test__/__snapshots__/UpdateBreadcrumbs.test.js.snap index 681c20d1424d..16eef69cbbb1 100644 --- a/x-pack/plugins/apm/public/components/app/Main/__test__/__snapshots__/UpdateBreadcrumbs.test.js.snap +++ b/x-pack/plugins/apm/public/components/app/Main/__test__/__snapshots__/UpdateBreadcrumbs.test.js.snap @@ -8,18 +8,7 @@ Array [ }, Object { "href": "#/opbeans-node?_g=myG&kuery=myKuery", - "text": , + "text": "opbeans-node", }, ] `; @@ -32,18 +21,7 @@ Array [ }, Object { "href": "#/opbeans-node?_g=myG&kuery=myKuery", - "text": , + "text": "opbeans-node", }, Object { "href": "#/opbeans-node/errors?_g=myG&kuery=myKuery", @@ -60,18 +38,7 @@ Array [ }, Object { "href": "#/opbeans-node?_g=myG&kuery=myKuery", - "text": , + "text": "opbeans-node", }, Object { "href": "#/opbeans-node/errors?_g=myG&kuery=myKuery", @@ -79,19 +46,7 @@ Array [ }, Object { "href": "#/opbeans-node/errors/myGroupId?_g=myG&kuery=myKuery", - "text": , + "text": "myGroupId", }, ] `; @@ -104,18 +59,7 @@ Array [ }, Object { "href": "#/opbeans-node?_g=myG&kuery=myKuery", - "text": , + "text": "opbeans-node", }, Object { "href": "#/opbeans-node/transactions?_g=myG&kuery=myKuery", @@ -132,18 +76,7 @@ Array [ }, Object { "href": "#/opbeans-node?_g=myG&kuery=myKuery", - "text": , + "text": "opbeans-node", }, Object { "href": "#/opbeans-node/transactions?_g=myG&kuery=myKuery", @@ -159,40 +92,16 @@ Array [ "text": "APM", }, Object { - "href": "#/:serviceName?_g=myG&kuery=myKuery", - "text": , + "href": "#/opbeans-node?_g=myG&kuery=myKuery", + "text": "opbeans-node", }, Object { - "href": "#/:serviceName/transactions?_g=myG&kuery=myKuery", + "href": "#/opbeans-node/transactions?_g=myG&kuery=myKuery", "text": "Transactions", }, Object { - "href": "#/:serviceName/transactions/request/my-transaction-name?_g=myG&kuery=myKuery", - "text": , + "href": "#/opbeans-node/transactions/request/my-transaction-name?_g=myG&kuery=myKuery", + "text": "my-transaction-name", }, ] `; diff --git a/x-pack/plugins/apm/public/components/app/Main/index.js b/x-pack/plugins/apm/public/components/app/Main/index.js index 865bf756f766..cbf1925e5650 100644 --- a/x-pack/plugins/apm/public/components/app/Main/index.js +++ b/x-pack/plugins/apm/public/components/app/Main/index.js @@ -25,17 +25,11 @@ export default function Main() { - {routes.map((route, i) => { - return route.switchRoutes ? ( - - {route.switchRoutes.map((route, i) => ( - - ))} - - ) : ( + + {routes.map((route, i) => ( - ); - })} + ))} + ); } diff --git a/x-pack/plugins/apm/public/components/app/Main/routeConfig.tsx b/x-pack/plugins/apm/public/components/app/Main/routeConfig.tsx index bbdb1076d942..d089dc5d8caa 100644 --- a/x-pack/plugins/apm/public/components/app/Main/routeConfig.tsx +++ b/x-pack/plugins/apm/public/components/app/Main/routeConfig.tsx @@ -6,32 +6,19 @@ import { i18n } from '@kbn/i18n'; import React from 'react'; -import { Redirect, RouteComponentProps, RouteProps } from 'react-router-dom'; +import { Redirect, RouteComponentProps } from 'react-router-dom'; import { legacyDecodeURIComponent } from 'x-pack/plugins/apm/public/components/shared/Links/url_helpers'; -import { StringMap } from '../../../../typings/common'; // @ts-ignore import ErrorGroupDetails from '../ErrorGroupDetails'; import { ServiceDetails } from '../ServiceDetails'; import { TransactionDetails } from '../TransactionDetails'; import { Home } from './Home'; - -interface BreadcrumbArgs { - match: { - params: StringMap; - }; -} +import { BreadcrumbRoute } from './ProvideBreadcrumbs'; interface RouteParams { serviceName: string; } -type BreadcrumbFunction = (args: BreadcrumbArgs) => string | null; - -interface Route extends RouteProps { - switchRoutes?: Route[]; - breadcrumb?: string | BreadcrumbFunction | null; -} - const renderAsRedirectTo = (to: string) => { return ({ location }: RouteComponentProps) => ( { ); }; -export const routes: Route[] = [ +export const routes: BreadcrumbRoute[] = [ { exact: true, path: '/', render: renderAsRedirectTo('/services'), breadcrumb: 'APM' }, + { + exact: true, + path: '/invalid-license', + breadcrumb: i18n.translate('xpack.apm.breadcrumb.invalidLicenseTitle', { + defaultMessage: 'Invalid License' + }), + render: () => ( +
+ {i18n.translate('xpack.apm.invalidLicenseLabel', { + defaultMessage: 'Invalid license' + })} +
+ ) + }, + { + exact: true, + path: '/services', + component: Home, + breadcrumb: i18n.translate('xpack.apm.breadcrumb.servicesTitle', { + defaultMessage: 'Services' + }) + }, + { + exact: true, + path: '/traces', + component: Home, + breadcrumb: i18n.translate('xpack.apm.breadcrumb.tracesTitle', { + defaultMessage: 'Traces' + }) + }, + { + exact: true, + path: '/:serviceName', + breadcrumb: ({ match }) => match.params.serviceName, + render: (props: RouteComponentProps) => + renderAsRedirectTo(`/${props.match.params.serviceName}/transactions`)( + props + ) + }, { exact: true, path: '/:serviceName/errors/:groupId', component: ErrorGroupDetails, - breadcrumb: ({ match }: BreadcrumbArgs) => match.params.groupId + breadcrumb: ({ match }) => match.params.groupId }, { exact: true, @@ -64,49 +90,6 @@ export const routes: Route[] = [ defaultMessage: 'Errors' }) }, - { - switchRoutes: [ - { - exact: true, - path: '/invalid-license', - breadcrumb: i18n.translate('xpack.apm.breadcrumb.invalidLicenseTitle', { - defaultMessage: 'Invalid License' - }), - render: () => ( -
- {i18n.translate('xpack.apm.invalidLicenseLabel', { - defaultMessage: 'Invalid license' - })} -
- ) - }, - { - exact: true, - path: '/services', - component: Home, - breadcrumb: i18n.translate('xpack.apm.breadcrumb.servicesTitle', { - defaultMessage: 'Services' - }) - }, - { - exact: true, - path: '/traces', - component: Home, - breadcrumb: i18n.translate('xpack.apm.breadcrumb.tracesTitle', { - defaultMessage: 'Traces' - }) - }, - { - exact: true, - path: '/:serviceName', - breadcrumb: ({ match }: BreadcrumbArgs) => match.params.serviceName, - render: (props: RouteComponentProps) => - renderAsRedirectTo(`/${props.match.params.serviceName}/transactions`)( - props - ) - } - ] - }, { exact: true, path: '/:serviceName/transactions', @@ -135,7 +118,7 @@ export const routes: Route[] = [ exact: true, path: '/:serviceName/transactions/:transactionType/:transactionName', component: TransactionDetails, - breadcrumb: ({ match }: BreadcrumbArgs) => - legacyDecodeURIComponent(match.params.transactionName) + breadcrumb: ({ match }) => + legacyDecodeURIComponent(match.params.transactionName) || '' } ]; diff --git a/yarn.lock b/yarn.lock index 9ccb841897ca..d964b2b26b58 100644 --- a/yarn.lock +++ b/yarn.lock @@ -17815,11 +17815,6 @@ react-resizable@1.x: prop-types "15.x" react-draggable "^2.2.6 || ^3.0.3" -react-router-breadcrumbs-hoc@1.1.2: - version "1.1.2" - resolved "https://registry.yarnpkg.com/react-router-breadcrumbs-hoc/-/react-router-breadcrumbs-hoc-1.1.2.tgz#4fafb620e7c6b876d98f7151f4c85ae5c3157dc0" - integrity sha1-T6+2IOfGuHbZj3FR9Mha5cMVfcA= - react-router-dom@4.2.2: version "4.2.2" resolved "https://registry.yarnpkg.com/react-router-dom/-/react-router-dom-4.2.2.tgz#c8a81df3adc58bba8a76782e946cbd4eae649b8d"