Cleanup from code review comments

elastic · May 28, 2021 · fd2e79b · fd2e79b
1 parent 311d7a8
commit fd2e79b
Show file tree

Hide file tree

Showing 11 changed files with 89 additions and 63 deletions.
diff --git 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
@@ -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()}...`,
+}) => (
   <div className="canvasContainer--loading">
     <EuiPanel>
       <EuiLoadingChart size="m" />

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
@@ -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 <Component onLoad={onLoad} />;
 };
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
@@ -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
@@ -12,6 +12,7 @@ import { useHistory } from 'react-router-dom';
 interface RoutingProps {
   to: string;
 }
+
 type RoutingLinkProps = Omit<EuiLinkProps, 'href' | 'onClick'> & RoutingProps;
 
 export const RoutingLink: FC<RoutingLinkProps> = ({ to, ...rest }) => {

diff --git 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 (
-    <Component
+    <WorkpadComponent
       {...props}
       setFullscreen={setFullscreenWithEffect}
       isFullscreen={isFullscreen}

diff --git a/...plugins/canvas/public/components/workpad_header/fullscreen_control/fullscreen_control.tsx b/...plugins/canvas/public/components/workpad_header/fullscreen_control/fullscreen_control.tsx
@@ -39,6 +39,12 @@ export class FullscreenControl extends React.PureComponent<Props> {
     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
@@ -99,7 +99,7 @@ export const KioskControls = ({ autoplayInterval, onSetInterval }: Props) => {
                     <EuiButtonIcon
                       iconType="cross"
                       onClick={disableAutoplay}
-                      aria-label={'something' || strings.getDisableTooltip()}
+                      aria-label={strings.getDisableTooltip()}
                     />
                   </EuiToolTip>
                 </EuiFlexItem>

diff --git a/x-pack/plugins/canvas/public/components/workpad_loader/index.tsx b/x-pack/plugins/canvas/public/components/workpad_loader/index.tsx
@@ -11,77 +11,79 @@ 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<ReturnType<typeof useServices>['workpad']['find']>;
+type WorkpadStatePromise = ReturnType<ReturnType<typeof useWorkpadService>['find']>;
 type WorkpadState = WorkpadStatePromise extends PromiseLike<infer U> ? 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<WorkpadState | null>(null);
-  const services = useServices();
+  const workpadService = useWorkpadService();
+  const notifyService = useNotifyService();
+  const platformService = usePlatformService();
   const history = useHistory();
 
   const createWorkpad = useCallback(
     async (_workpad: CanvasWorkpad | null | undefined) => {
       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), []);
 
   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 (
     <Component
-      workpadId={fromState.workpadId}
-      canUserWrite={fromState.canUserWrite}
-      cloneWorkpad={cloneWorkpad}
-      createWorkpad={createWorkpad}
-      findWorkpads={findWorkpads}
-      downloadWorkpad={onDownloadWorkpad}
-      removeWorkpads={removeWorkpads}
-      onClose={onClose}
-      workpads={workpadsState}
-      formatDate={formatDate}
+      {...{
+        downloadWorkpad: onDownloadWorkpad,
+        workpads: workpadsState,
+        workpadId,
+        canUserWrite,
+        cloneWorkpad,
+        createWorkpad,
+        findWorkpads,
+        removeWorkpads,
+        formatDate,
+        onClose,
+      }}
     />
   );
 };
diff --git 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 = () => (
   <Route
     path={'/workpad/:id'}
     exact={false}
-    children={(route: WorkpadRouteProps) => {
-      return [
-        <WorkpadLoaderComponent params={route.match.params} key="workpad-loader">
-          {(workpad: CanvasWorkpad) => (
-            <Switch>
-              <Route
-                path="/workpad/:id/page/:pageNumber"
-                children={(pageRoute) => (
-                  <WorkpadHistoryManager>
-                    <WorkpadRoutingContextComponent>
-                      <WorkpadPresentationHelper>
-                        <WorkpadApp />
-                      </WorkpadPresentationHelper>
-                    </WorkpadRoutingContextComponent>
-                  </WorkpadHistoryManager>
-                )}
-              />
-              <Route path="/workpad/:id" strict={false} exact={true}>
-                <Redirect to={`/workpad/${route.match.params.id}/page/${workpad.page + 1}`} />
-              </Route>
-            </Switch>
-          )}
-        </WorkpadLoaderComponent>,
-      ];
-    }}
+    children={(route: WorkpadRouteProps) => (
+      <WorkpadLoaderComponent params={route.match.params} key="workpad-loader">
+        {(workpad: CanvasWorkpad) => (
+          <Switch>
+            <Route
+              path="/workpad/:id/page/:pageNumber"
+              children={(pageRoute) => (
+                <WorkpadHistoryManager>
+                  <WorkpadRoutingContextComponent>
+                    <WorkpadPresentationHelper>
+                      <WorkpadApp />
+                    </WorkpadPresentationHelper>
+                  </WorkpadRoutingContextComponent>
+                </WorkpadHistoryManager>
+              )}
+            />
+            <Route path="/workpad/:id" strict={false} exact={true}>
+              <Redirect to={`/workpad/${route.match.params.id}/page/${workpad.page + 1}`} />
+            </Route>
+          </Switch>
+        )}
+      </WorkpadLoaderComponent>
+    )}
   />
 );
 

diff --git 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<CanvasServices>(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 = <Props extends WithServicesProps>(type: ComponentType<Props>) => {
   const EnhancedType: FC<Props> = (props) =>