From fd2e79b3e6103394233575c665a0a6a704f98927 Mon Sep 17 00:00:00 2001 From: Corey Robertson Date: Fri, 28 May 2021 16:09:11 -0400 Subject: [PATCH] Cleanup from code review comments --- x-pack/plugins/canvas/i18n/components.ts | 6 ++ .../canvas_loading.component.tsx | 7 +- .../public/components/home_app/home_app.tsx | 8 +-- .../components/page_manager/page_manager.tsx | 2 + .../components/routing/routing_link.tsx | 1 + .../canvas/public/components/workpad/index.js | 4 +- .../fullscreen_control/fullscreen_control.tsx | 6 ++ .../view_menu/kiosk_controls.tsx | 2 +- .../components/workpad_loader/index.tsx | 66 ++++++++++--------- .../public/routes/workpad/workpad_route.tsx | 48 +++++++------- .../canvas/public/services/context.tsx | 2 + 11 files changed, 89 insertions(+), 63 deletions(-) diff --git a/x-pack/plugins/canvas/i18n/components.ts b/x-pack/plugins/canvas/i18n/components.ts index 9b09fe809c4a7..a797a8bda061b 100644 --- a/x-pack/plugins/canvas/i18n/components.ts +++ b/x-pack/plugins/canvas/i18n/components.ts @@ -175,6 +175,12 @@ export const ComponentStrings = { defaultMessage: 'Asset thumbnail', }), }, + CanvasLoading: { + getLoadingLabel: () => + i18n.translate('xpack.canvas.canvasLoading.loadingMessage', { + defaultMessage: 'Loading', + }), + }, ColorManager: { getAddAriaLabel: () => i18n.translate('xpack.canvas.colorManager.addAriaLabel', { diff --git a/x-pack/plugins/canvas/public/components/canvas_loading/canvas_loading.component.tsx b/x-pack/plugins/canvas/public/components/canvas_loading/canvas_loading.component.tsx index 17d1391497f51..38e62f46c945a 100644 --- a/x-pack/plugins/canvas/public/components/canvas_loading/canvas_loading.component.tsx +++ b/x-pack/plugins/canvas/public/components/canvas_loading/canvas_loading.component.tsx @@ -7,8 +7,13 @@ import React, { FC } from 'react'; import { EuiPanel, EuiLoadingChart, EuiSpacer, EuiText } from '@elastic/eui'; +import { ComponentStrings } from '../../../i18n/components'; -export const CanvasLoading: FC<{ msg?: string }> = ({ msg = 'Loading...' }) => ( +const { CanvasLoading: strings } = ComponentStrings; + +export const CanvasLoading: FC<{ msg?: string }> = ({ + msg = `${strings.getLoadingLabel()}...`, +}) => (
diff --git a/x-pack/plugins/canvas/public/components/home_app/home_app.tsx b/x-pack/plugins/canvas/public/components/home_app/home_app.tsx index fde448a79bf1e..b288612450bf7 100644 --- a/x-pack/plugins/canvas/public/components/home_app/home_app.tsx +++ b/x-pack/plugins/canvas/public/components/home_app/home_app.tsx @@ -10,16 +10,16 @@ import { useDispatch } from 'react-redux'; import { getBaseBreadcrumb } from '../../lib/breadcrumbs'; import { resetWorkpad } from '../../state/actions/workpad'; import { HomeApp as Component } from './home_app.component'; -import { useServices } from '../../services'; +import { usePlatformService } from '../../services'; export const HomeApp = () => { - const services = useServices(); + const { setBreadcrumbs } = usePlatformService(); const dispatch = useDispatch(); const onLoad = () => dispatch(resetWorkpad()); useEffect(() => { - services.platform.setBreadcrumbs([getBaseBreadcrumb()]); - }, [services.platform]); + setBreadcrumbs([getBaseBreadcrumb()]); + }, [setBreadcrumbs]); return ; }; diff --git a/x-pack/plugins/canvas/public/components/page_manager/page_manager.tsx b/x-pack/plugins/canvas/public/components/page_manager/page_manager.tsx index 53aa68eb51d09..5452fe8eae295 100644 --- a/x-pack/plugins/canvas/public/components/page_manager/page_manager.tsx +++ b/x-pack/plugins/canvas/public/components/page_manager/page_manager.tsx @@ -32,10 +32,12 @@ export const PageManager: FC<{ onPreviousPage: () => void }> = ({ onPreviousPage dispatch, gotoPage, ]); + const onMovePage = useCallback( (id: string, position: number) => dispatch(pageActions.movePage(id, position, gotoPage)), [dispatch, gotoPage] ); + const onRemovePage = useCallback( (id: string) => dispatch(pageActions.removePage({ id, gotoPage })), [dispatch, gotoPage] diff --git a/x-pack/plugins/canvas/public/components/routing/routing_link.tsx b/x-pack/plugins/canvas/public/components/routing/routing_link.tsx index 773fdb1ad6cb7..bb3123de3fec8 100644 --- a/x-pack/plugins/canvas/public/components/routing/routing_link.tsx +++ b/x-pack/plugins/canvas/public/components/routing/routing_link.tsx @@ -12,6 +12,7 @@ import { useHistory } from 'react-router-dom'; interface RoutingProps { to: string; } + type RoutingLinkProps = Omit & RoutingProps; export const RoutingLink: FC = ({ to, ...rest }) => { diff --git a/x-pack/plugins/canvas/public/components/workpad/index.js b/x-pack/plugins/canvas/public/components/workpad/index.js index 0a9e3e176a458..2ebefa5e0e322 100644 --- a/x-pack/plugins/canvas/public/components/workpad/index.js +++ b/x-pack/plugins/canvas/public/components/workpad/index.js @@ -22,7 +22,7 @@ import { zoomHandlerCreators } from '../../lib/app_handler_creators'; import { trackCanvasUiMetric, METRIC_TYPE } from '../../lib/ui_metric'; import { LAUNCHED_FULLSCREEN, LAUNCHED_FULLSCREEN_AUTOPLAY } from '../../../common/lib/constants'; import { WorkpadRoutingContext } from '../../routes/workpad'; -import { Workpad as Component } from './workpad'; +import { Workpad as WorkpadComponent } from './workpad'; const mapStateToProps = (state) => { const { width, height, id: workpadId, css: workpadCss } = getWorkpad(state); @@ -73,7 +73,7 @@ const AddContexts = (props) => { ); return ( - { children: PropTypes.func.isRequired, }; + /* + We need these instance functions because ReactShortcuts bind the handlers on it's mount, + but then does no rebinding if it's props change. Using these instance functions will + properly handle changes to incoming props since the instance functions are bound to the components + "this" context + */ _toggleFullscreen = () => { const { setFullscreen, isFullscreen } = this.props; setFullscreen(!isFullscreen); diff --git a/x-pack/plugins/canvas/public/components/workpad_header/view_menu/kiosk_controls.tsx b/x-pack/plugins/canvas/public/components/workpad_header/view_menu/kiosk_controls.tsx index 9247b461047c6..55373d7a3515c 100644 --- a/x-pack/plugins/canvas/public/components/workpad_header/view_menu/kiosk_controls.tsx +++ b/x-pack/plugins/canvas/public/components/workpad_header/view_menu/kiosk_controls.tsx @@ -99,7 +99,7 @@ export const KioskControls = ({ autoplayInterval, onSetInterval }: Props) => { diff --git a/x-pack/plugins/canvas/public/components/workpad_loader/index.tsx b/x-pack/plugins/canvas/public/components/workpad_loader/index.tsx index fe5aae4577fa9..2afd5fe70abe1 100644 --- a/x-pack/plugins/canvas/public/components/workpad_loader/index.tsx +++ b/x-pack/plugins/canvas/public/components/workpad_loader/index.tsx @@ -11,30 +11,32 @@ import { useSelector } from 'react-redux'; import moment from 'moment'; // @ts-expect-error import { getDefaultWorkpad } from '../../state/defaults'; -import { canUserWrite } from '../../state/selectors/app'; +import { canUserWrite as canUserWriteSelector } from '../../state/selectors/app'; import { getWorkpad } from '../../state/selectors/workpad'; import { getId } from '../../lib/get_id'; import { downloadWorkpad } from '../../lib/download_workpad'; import { ComponentStrings, ErrorStrings } from '../../../i18n'; import { State, CanvasWorkpad } from '../../../types'; -import { useServices } from '../../services'; +import { useNotifyService, useWorkpadService, usePlatformService } from '../../services'; // @ts-expect-error import { WorkpadLoader as Component } from './workpad_loader'; const { WorkpadLoader: strings } = ComponentStrings; const { WorkpadLoader: errors } = ErrorStrings; -type WorkpadStatePromise = ReturnType['workpad']['find']>; +type WorkpadStatePromise = ReturnType['find']>; type WorkpadState = WorkpadStatePromise extends PromiseLike ? U : never; export const WorkpadLoader: FC<{ onClose: () => void }> = ({ onClose }) => { const fromState = useSelector((state: State) => ({ workpadId: getWorkpad(state).id, - canUserWrite: canUserWrite(state), + canUserWrite: canUserWriteSelector(state), })); const [workpadsState, setWorkpadsState] = useState(null); - const services = useServices(); + const workpadService = useWorkpadService(); + const notifyService = useNotifyService(); + const platformService = usePlatformService(); const history = useHistory(); const createWorkpad = useCallback( @@ -42,29 +44,29 @@ export const WorkpadLoader: FC<{ onClose: () => void }> = ({ onClose }) => { const workpad = _workpad || getDefaultWorkpad(); if (workpad != null) { try { - await services.workpad.create(workpad); + await workpadService.create(workpad); history.push(`/workpad/${workpad.id}/page/1`); } catch (err) { - services.notify.error(err, { + notifyService.error(err, { title: errors.getUploadFailureErrorMessage(), }); } return; } }, - [services.workpad, services.notify, history] + [workpadService, notifyService, history] ); const findWorkpads = useCallback( async (text) => { try { - const fetchedWorkpads = await services.workpad.find(text); + const fetchedWorkpads = await workpadService.find(text); setWorkpadsState(fetchedWorkpads); } catch (err) { - services.notify.error(err, { title: errors.getFindFailureErrorMessage() }); + notifyService.error(err, { title: errors.getFindFailureErrorMessage() }); } }, - [services.notify, services.workpad] + [notifyService, workpadService] ); const onDownloadWorkpad = useCallback((workpadId: string) => downloadWorkpad(workpadId), []); @@ -72,16 +74,16 @@ export const WorkpadLoader: FC<{ onClose: () => void }> = ({ onClose }) => { const cloneWorkpad = useCallback( async (workpadId: string) => { try { - const workpad = await services.workpad.get(workpadId); + const workpad = await workpadService.get(workpadId); workpad.name = strings.getClonedWorkpadName(workpad.name); workpad.id = getId('workpad'); - await services.workpad.create(workpad); + await workpadService.create(workpad); history.push(`/workpad/${workpad.id}/page/1`); } catch (err) { - services.notify.error(err, { title: errors.getCloneFailureErrorMessage() }); + notifyService.error(err, { title: errors.getCloneFailureErrorMessage() }); } }, - [services.notify, services.workpad, history] + [notifyService, workpadService, history] ); const removeWorkpads = useCallback( @@ -92,7 +94,7 @@ export const WorkpadLoader: FC<{ onClose: () => void }> = ({ onClose }) => { const removedWorkpads = workpadIds.map(async (id) => { try { - await services.workpad.remove(id); + await workpadService.remove(id); return { id, err: null }; } catch (err) { return { id, err }; @@ -127,7 +129,7 @@ export const WorkpadLoader: FC<{ onClose: () => void }> = ({ onClose }) => { }; if (errored.length > 0) { - services.notify.error(errors.getDeleteFailureErrorMessage()); + notifyService.error(errors.getDeleteFailureErrorMessage()); } setWorkpadsState(workpadState); @@ -139,29 +141,33 @@ export const WorkpadLoader: FC<{ onClose: () => void }> = ({ onClose }) => { return errored; }); }, - [history, services.workpad, fromState.workpadId, workpadsState, services.notify] + [history, workpadService, fromState.workpadId, workpadsState, notifyService] ); const formatDate = useCallback( (date: any) => { - const dateFormat = services.platform.getUISetting('dateFormat'); + const dateFormat = platformService.getUISetting('dateFormat'); return date && moment(date).format(dateFormat); }, - [services.platform] + [platformService] ); + const { workpadId, canUserWrite } = fromState; + return ( ); }; diff --git a/x-pack/plugins/canvas/public/routes/workpad/workpad_route.tsx b/x-pack/plugins/canvas/public/routes/workpad/workpad_route.tsx index f302fa4f4d623..7683b3413f681 100644 --- a/x-pack/plugins/canvas/public/routes/workpad/workpad_route.tsx +++ b/x-pack/plugins/canvas/public/routes/workpad/workpad_route.tsx @@ -30,31 +30,29 @@ export const WorkpadRoute = () => ( { - return [ - - {(workpad: CanvasWorkpad) => ( - - ( - - - - - - - - )} - /> - - - - - )} - , - ]; - }} + children={(route: WorkpadRouteProps) => ( + + {(workpad: CanvasWorkpad) => ( + + ( + + + + + + + + )} + /> + + + + + )} + + )} /> ); diff --git a/x-pack/plugins/canvas/public/services/context.tsx b/x-pack/plugins/canvas/public/services/context.tsx index 15885c01f4137..3a78e314b9635 100644 --- a/x-pack/plugins/canvas/public/services/context.tsx +++ b/x-pack/plugins/canvas/public/services/context.tsx @@ -26,6 +26,7 @@ const defaultContextValue = { platform: {}, navLink: {}, search: {}, + workpad: {}, }; const context = createContext(defaultContextValue as CanvasServices); @@ -37,6 +38,7 @@ export const useExpressionsService = () => useServices().expressions; export const useNotifyService = () => useServices().notify; export const useNavLinkService = () => useServices().navLink; export const useLabsService = () => useServices().labs; +export const useWorkpadService = () => useServices().workpad; export const withServices = (type: ComponentType) => { const EnhancedType: FC = (props) =>