From f8cf5dd6d5ee91ab0354d350ff9b03c718dfe6e1 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Fri, 9 Apr 2021 11:20:16 -0700 Subject: [PATCH] Refactor ResultActions component + DRY out link behavior - Create new separate ResultActions component - Pass actions array through to header and have haeder in charge of conditional visibility / FlexItem wrapper (this matches the other header items) - shouldLinkToDetailPage: instead of generating custom JSX, just have it be a standard action and append it to the actions array Link behavior: - ResultHeaderItem - switch to EuiLinkTo, no need for extra wrapper - ResultHeader - DRY out unnecessary extra path generation - instead pass down a conditional documentLink instead of a bool --- .../components/result/result.test.tsx | 83 ++++++++----------- .../app_search/components/result/result.tsx | 61 +++++--------- .../components/result/result_actions.test.tsx | 55 ++++++++++++ .../components/result/result_actions.tsx | 34 ++++++++ .../components/result/result_header.test.tsx | 68 +++++++-------- .../components/result/result_header.tsx | 25 +++--- .../result/result_header_item.test.tsx | 4 +- .../components/result/result_header_item.tsx | 10 +-- 8 files changed, 189 insertions(+), 151 deletions(-) create mode 100644 x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_actions.test.tsx create mode 100644 x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_actions.tsx diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.test.tsx index 3e83717bf93559..ba9944744e5c79 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.test.tsx @@ -5,12 +5,14 @@ * 2.0. */ +import { mockKibanaValues } from '../../../__mocks__'; + import React from 'react'; import { DraggableProvidedDragHandleProps } from 'react-beautiful-dnd'; import { shallow, ShallowWrapper } from 'enzyme'; -import { EuiButtonIcon, EuiPanel, EuiButtonIconColor } from '@elastic/eui'; +import { EuiPanel } from '@elastic/eui'; import { SchemaTypes } from '../../../shared/types'; @@ -63,18 +65,28 @@ describe('Result', () => { ]); }); - it('renders a header', () => { - const wrapper = shallow(); - const header = wrapper.find(ResultHeader); - expect(header.exists()).toBe(true); - expect(header.prop('isMetaEngine')).toBe(true); // passed through from props - expect(header.prop('showScore')).toBe(true); // passed through from props - expect(header.prop('shouldLinkToDetailPage')).toBe(false); // passed through from props - expect(header.prop('resultMeta')).toEqual({ - id: '1', - score: 100, - engine: 'my-engine', - }); // passed through from meta in result prop + describe('header', () => { + it('renders a header', () => { + const wrapper = shallow(); + const header = wrapper.find(ResultHeader); + + expect(header.exists()).toBe(true); + expect(header.prop('isMetaEngine')).toBe(true); // passed through from props + expect(header.prop('showScore')).toBe(true); // passed through from props + expect(header.prop('resultMeta')).toEqual({ + id: '1', + score: 100, + engine: 'my-engine', + }); // passed through from meta in result prop + expect(header.prop('documentLink')).toBe(undefined); // based on shouldLinkToDetailPage prop + }); + + it('passes documentLink when shouldLinkToDetailPage is true', () => { + const wrapper = shallow(); + const header = wrapper.find(ResultHeader); + + expect(header.prop('documentLink')).toBe('/engines/my-engine/documents/1'); + }); }); describe('actions', () => { @@ -83,53 +95,30 @@ describe('Result', () => { title: 'Hide', onClick: jest.fn(), iconType: 'eyeClosed', - iconColor: 'danger' as EuiButtonIconColor, }, { title: 'Bookmark', onClick: jest.fn(), iconType: 'starFilled', - iconColor: undefined, }, ]; - it('will render an action button in the header for each action passed', () => { + it('passes actions to the header', () => { const wrapper = shallow(); - const header = wrapper.find(ResultHeader); - const renderedActions = shallow(header.prop('actions') as any); - const buttons = renderedActions.find(EuiButtonIcon); - expect(buttons).toHaveLength(2); - - expect(buttons.first().prop('iconType')).toEqual('eyeClosed'); - expect(buttons.first().prop('color')).toEqual('danger'); - buttons.first().simulate('click'); - expect(actions[0].onClick).toHaveBeenCalled(); - - expect(buttons.last().prop('iconType')).toEqual('starFilled'); - // Note that no iconColor was passed so it was defaulted to primary - expect(buttons.last().prop('color')).toEqual('primary'); - buttons.last().simulate('click'); - expect(actions[1].onClick).toHaveBeenCalled(); + expect(wrapper.find(ResultHeader).prop('actions')).toEqual(actions); }); - it('will render a document detail link as the first action if shouldLinkToDetailPage is passed', () => { + it('adds a link action to the start of the actions array if shouldLinkToDetailPage is passed', () => { const wrapper = shallow(); - const header = wrapper.find(ResultHeader); - const renderedActions = shallow(header.prop('actions') as any); - const buttons = renderedActions.find(EuiButtonIcon); - // In addition to the 2 actions passed, we also have a link action - expect(buttons).toHaveLength(3); + const passedActions = wrapper.find(ResultHeader).prop('actions'); + expect(passedActions.length).toEqual(3); // In addition to the 2 actions passed, we also have a link action - expect(buttons.first().prop('data-test-subj')).toEqual('DocumentDetailLink'); - }); + const linkAction = passedActions[0]; + expect(linkAction.title).toEqual('Visit document details'); - it('will not render anything if no actions are passed and shouldLinkToDetailPage is false', () => { - const wrapper = shallow(); - const header = wrapper.find(ResultHeader); - const renderedActions = shallow(header.prop('actions') as any); - const buttons = renderedActions.find(EuiButtonIcon); - expect(buttons).toHaveLength(0); + linkAction.onClick(); + expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/engines/my-engine/documents/1'); }); }); @@ -148,9 +137,7 @@ describe('Result', () => { }); it('will render field details with type highlights if schemaForTypeHighlights has been provided', () => { - const wrapper = shallow( - - ); + const wrapper = shallow(); expect(wrapper.find(ResultField).map((rf) => rf.prop('type'))).toEqual([ 'text', 'text', diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx index 71d9f39d802d5b..f1dc408c2470f2 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx @@ -10,12 +10,11 @@ import { DraggableProvidedDragHandleProps } from 'react-beautiful-dnd'; import './result.scss'; -import { EuiButtonIcon, EuiPanel, EuiFlexGroup, EuiFlexItem, EuiIcon } from '@elastic/eui'; +import { EuiPanel, EuiIcon } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; -import { ReactRouterHelper } from '../../../shared/react_router_helpers/eui_components'; - +import { KibanaLogic } from '../../../shared/kibana'; import { Schema } from '../../../shared/types'; import { ENGINE_DOCUMENT_DETAIL_PATH } from '../../routes'; @@ -56,48 +55,26 @@ export const Result: React.FC = ({ [result] ); const numResults = resultFields.length; - const documentLink = generateEncodedPath(ENGINE_DOCUMENT_DETAIL_PATH, { - engineName: resultMeta.engine, - documentId: resultMeta.id, - }); const typeForField = (fieldName: string) => { if (schemaForTypeHighlights) return schemaForTypeHighlights[fieldName]; }; - const ResultActions = () => { - if (!shouldLinkToDetailPage && !actions.length) return null; - return ( - - - {shouldLinkToDetailPage && ( - - - - - - )} - {actions.map(({ onClick, title, iconType, iconColor }) => ( - - - - ))} - - - ); - }; + const documentLink = shouldLinkToDetailPage + ? generateEncodedPath(ENGINE_DOCUMENT_DETAIL_PATH, { + engineName: resultMeta.engine, + documentId: resultMeta.id, + }) + : undefined; + if (shouldLinkToDetailPage && documentLink) { + actions.unshift({ + onClick: () => KibanaLogic.values.navigateToUrl(documentLink), + title: i18n.translate('xpack.enterpriseSearch.appSearch.result.documentDetailLink', { + defaultMessage: 'Visit document details', + }), + iconType: 'eye', + }); + } return ( = ({ resultMeta={resultMeta} showScore={!!showScore} isMetaEngine={isMetaEngine} - shouldLinkToDetailPage={shouldLinkToDetailPage} - actions={} + documentLink={documentLink} + actions={actions} /> {resultFields .slice(0, isOpen ? resultFields.length : RESULT_CUTOFF) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_actions.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_actions.test.tsx new file mode 100644 index 00000000000000..4aae1e07f0f8c5 --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_actions.test.tsx @@ -0,0 +1,55 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import React from 'react'; + +import { shallow } from 'enzyme'; + +import { EuiButtonIcon, EuiButtonIconColor } from '@elastic/eui'; + +import { ResultActions } from './result_actions'; + +describe('ResultActions', () => { + const actions = [ + { + title: 'Hide', + onClick: jest.fn(), + iconType: 'eyeClosed', + iconColor: 'danger' as EuiButtonIconColor, + }, + { + title: 'Bookmark', + onClick: jest.fn(), + iconType: 'starFilled', + iconColor: undefined, + }, + ]; + + const wrapper = shallow(); + const buttons = wrapper.find(EuiButtonIcon); + + it('renders an action button for each action passed', () => { + expect(buttons).toHaveLength(2); + }); + + it('passes icon props correctly', () => { + expect(buttons.first().prop('iconType')).toEqual('eyeClosed'); + expect(buttons.first().prop('color')).toEqual('danger'); + + expect(buttons.last().prop('iconType')).toEqual('starFilled'); + // Note that no iconColor was passed so it was defaulted to primary + expect(buttons.last().prop('color')).toEqual('primary'); + }); + + it('passes click events', () => { + buttons.first().simulate('click'); + expect(actions[0].onClick).toHaveBeenCalled(); + + buttons.last().simulate('click'); + expect(actions[1].onClick).toHaveBeenCalled(); + }); +}); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_actions.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_actions.tsx new file mode 100644 index 00000000000000..52fbee90fe31af --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_actions.tsx @@ -0,0 +1,34 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import React from 'react'; + +import { EuiButtonIcon, EuiFlexGroup, EuiFlexItem } from '@elastic/eui'; + +import { ResultAction } from './types'; + +interface Props { + actions: ResultAction[]; +} + +export const ResultActions: React.FC = ({ actions }) => { + return ( + + {actions.map(({ onClick, title, iconType, iconColor }) => ( + + + + ))} + + ); +}; diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header.test.tsx index 80cff9b96a3cad..e063cbc067a631 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header.test.tsx @@ -9,6 +9,7 @@ import React from 'react'; import { shallow } from 'enzyme'; +import { ResultActions } from './result_actions'; import { ResultHeader } from './result_header'; describe('ResultHeader', () => { @@ -17,30 +18,27 @@ describe('ResultHeader', () => { score: 100, engine: 'my-engine', }; + const props = { + showScore: false, + isMetaEngine: false, + resultMeta, + actions: [], + }; it('renders', () => { - const wrapper = shallow( - - ); + const wrapper = shallow(); expect(wrapper.isEmptyRender()).toBe(false); }); it('always renders an id', () => { - const wrapper = shallow( - - ); + const wrapper = shallow(); expect(wrapper.find('[data-test-subj="ResultId"]').prop('value')).toEqual('1'); expect(wrapper.find('[data-test-subj="ResultId"]').prop('href')).toBeUndefined(); }); - it('renders id as a link if shouldLinkToDetailPage is true', () => { + it('renders id as a link if a documentLink has been passed', () => { const wrapper = shallow( - + ); expect(wrapper.find('[data-test-subj="ResultId"]').prop('value')).toEqual('1'); expect(wrapper.find('[data-test-subj="ResultId"]').prop('href')).toEqual( @@ -50,47 +48,39 @@ describe('ResultHeader', () => { describe('score', () => { it('renders score if showScore is true ', () => { - const wrapper = shallow( - - ); + const wrapper = shallow(); expect(wrapper.find('[data-test-subj="ResultScore"]').prop('value')).toEqual(100); }); it('does not render score if showScore is false', () => { - const wrapper = shallow( - - ); + const wrapper = shallow(); expect(wrapper.find('[data-test-subj="ResultScore"]').exists()).toBe(false); }); }); describe('engine', () => { it('renders engine name if this is a meta engine', () => { - const wrapper = shallow( - - ); + const wrapper = shallow(); expect(wrapper.find('[data-test-subj="ResultEngine"]').prop('value')).toBe('my-engine'); }); it('does not render an engine if this is not a meta engine', () => { - const wrapper = shallow( - - ); + const wrapper = shallow(); expect(wrapper.find('[data-test-subj="ResultEngine"]').exists()).toBe(false); }); }); + + describe('actions', () => { + const actions = [{ title: 'View document', onClick: () => {}, iconType: 'eye' }]; + + it('renders ResultActions if actions have been passed', () => { + const wrapper = shallow(); + expect(wrapper.find(ResultActions).exists()).toBe(true); + }); + + it('does not render an', () => { + const wrapper = shallow(); + expect(wrapper.find(ResultActions).exists()).toBe(false); + }); + }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header.tsx index 6f95c166153dff..f577b481b39cf3 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header.tsx @@ -9,11 +9,9 @@ import React from 'react'; import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui'; -import { ENGINE_DOCUMENT_DETAIL_PATH } from '../../routes'; -import { generateEncodedPath } from '../../utils/encode_path_params'; - +import { ResultActions } from './result_actions'; import { ResultHeaderItem } from './result_header_item'; -import { ResultMeta } from './types'; +import { ResultMeta, ResultAction } from './types'; import './result_header.scss'; @@ -21,8 +19,8 @@ interface Props { showScore: boolean; isMetaEngine: boolean; resultMeta: ResultMeta; - actions?: React.ReactNode; - shouldLinkToDetailPage?: boolean; + actions: ResultAction[]; + documentLink?: string; } export const ResultHeader: React.FC = ({ @@ -30,13 +28,8 @@ export const ResultHeader: React.FC = ({ resultMeta, isMetaEngine, actions, - shouldLinkToDetailPage = false, + documentLink, }) => { - const documentLink = generateEncodedPath(ENGINE_DOCUMENT_DETAIL_PATH, { - engineName: resultMeta.engine, - documentId: resultMeta.id, - }); - return (
= ({ > = ({ /> )} - {actions} + {actions.length > 0 && ( + + + + )}
); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header_item.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header_item.test.tsx index e0407b4db7f251..d45eb8856d118b 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header_item.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header_item.test.tsx @@ -69,7 +69,7 @@ describe('ResultHeaderItem', () => { const wrapper = shallow( ); - expect(wrapper.find('ReactRouterHelper').exists()).toBe(true); - expect(wrapper.find('ReactRouterHelper').prop('to')).toBe('http://www.example.com'); + expect(wrapper.find('EuiLinkTo').exists()).toBe(true); + expect(wrapper.find('EuiLinkTo').prop('to')).toBe('http://www.example.com'); }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header_item.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header_item.tsx index 545b85c17a529d..cf3b385fd92577 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header_item.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result_header_item.tsx @@ -9,7 +9,7 @@ import React from 'react'; import './result_header_item.scss'; -import { ReactRouterHelper } from '../../../shared/react_router_helpers/eui_components'; +import { EuiLinkTo } from '../../../shared/react_router_helpers/eui_components'; import { TruncatedContent } from '../../../shared/truncate'; @@ -48,11 +48,9 @@ export const ResultHeaderItem: React.FC = ({ field, type, value, href })   {href ? ( - - - - - + + + ) : ( )}