chore: Enhance Omnibar (apache#16273)

* Fix style and implement ESC * Include ESC test case * Move pagination outside of table * Update superset-frontend/src/components/OmniContainer/OmniContainer.test.tsx Co-authored-by: Evan Rusackas <[email protected]> * Enhance * Handle close * Localize placeholder * Fix tests * Clear input on close * Destroy modal on close * Clean up * Fix tests Co-authored-by: Evan Rusackas <[email protected]>
opus-42 · Nov 14, 2021 · 3f40d81 · 3f40d81
1 parent 9875959
commit 3f40d81
Show file tree

Hide file tree

Showing 6 changed files with 54 additions and 36 deletions.
diff --git a/superset-frontend/src/components/Modal/Modal.tsx b/superset-frontend/src/components/Modal/Modal.tsx
@@ -46,6 +46,7 @@ export interface ModalProps {
   wrapProps?: object;
   height?: string;
   closable?: boolean;
+  destroyOnClose?: boolean;
 }
 
 interface StyledModalProps {

diff --git a/superset-frontend/src/components/OmniContainer/OmniContainer.test.tsx b/superset-frontend/src/components/OmniContainer/OmniContainer.test.tsx
@@ -31,10 +31,9 @@ test('Do not open Omnibar with the featureflag disabled', () => {
   (isFeatureEnabled as jest.Mock).mockImplementation(
     (ff: string) => !(ff === 'OMNIBAR'),
   );
-  const logEvent = jest.fn();
   render(
     <div data-test="test">
-      <OmniContainer logEvent={logEvent} />
+      <OmniContainer />
     </div>,
   );
 
@@ -54,10 +53,9 @@ test('Open Omnibar with ctrl + k with featureflag enabled', () => {
   (isFeatureEnabled as jest.Mock).mockImplementation(
     (ff: string) => ff === 'OMNIBAR',
   );
-  const logEvent = jest.fn();
   render(
     <div data-test="test">
-      <OmniContainer logEvent={logEvent} />
+      <OmniContainer />
     </div>,
   );
 
@@ -81,17 +79,16 @@ test('Open Omnibar with ctrl + k with featureflag enabled', () => {
   });
   expect(
     screen.queryByPlaceholderText('Search all dashboards'),
-  ).not.toBeVisible();
+  ).not.toBeInTheDocument();
 });
 
 test('Open Omnibar with Command + k with featureflag enabled', () => {
   (isFeatureEnabled as jest.Mock).mockImplementation(
     (ff: string) => ff === 'OMNIBAR',
   );
-  const logEvent = jest.fn();
   render(
     <div data-test="test">
-      <OmniContainer logEvent={logEvent} />
+      <OmniContainer />
     </div>,
   );
 
@@ -115,17 +112,16 @@ test('Open Omnibar with Command + k with featureflag enabled', () => {
   });
   expect(
     screen.queryByPlaceholderText('Search all dashboards'),
-  ).not.toBeVisible();
+  ).not.toBeInTheDocument();
 });
 
 test('Open Omnibar with Cmd/Ctrl-K and close with ESC', () => {
   (isFeatureEnabled as jest.Mock).mockImplementation(
     (ff: string) => ff === 'OMNIBAR',
   );
-  const logEvent = jest.fn();
   render(
     <div data-test="test">
-      <OmniContainer logEvent={logEvent} />
+      <OmniContainer />
     </div>,
   );
 
@@ -149,5 +145,5 @@ test('Open Omnibar with Cmd/Ctrl-K and close with ESC', () => {
   });
   expect(
     screen.queryByPlaceholderText('Search all dashboards'),
-  ).not.toBeVisible();
+  ).not.toBeInTheDocument();
 });
diff --git a/superset-frontend/src/components/OmniContainer/Omnibar.tsx b/superset-frontend/src/components/OmniContainer/Omnibar.tsx
@@ -38,7 +38,8 @@ export function Omnibar({ extensions, placeholder, id }: Props) {
       id={id}
       placeholder={placeholder}
       extensions={extensions}
-      // autoFocus // I tried to use this prop (autoFocus) but it only works the first time that Omnibar is shown
+      autoComplete="off"
+      autoFocus
     />
   );
 }
diff --git a/superset-frontend/src/components/OmniContainer/index.tsx b/superset-frontend/src/components/OmniContainer/index.tsx
@@ -18,10 +18,11 @@
  */
 
 import React, { useRef, useState } from 'react';
-import { styled } from '@superset-ui/core';
+import { styled, t } from '@superset-ui/core';
 import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
 import Modal from 'src/components/Modal';
 import { useComponentDidMount } from 'src/common/hooks/useComponentDidMount';
+import { logEvent } from 'src/logger/actions';
 import { Omnibar } from './Omnibar';
 import { LOG_ACTIONS_OMNIBAR_TRIGGERED } from '../../logger/LogUtils';
 import { getDashboards } from './getDashboards';
@@ -35,43 +36,55 @@ const OmniModal = styled(Modal)`
   }
 `;
 
-interface Props {
-  logEvent: (log: string, object: object) => void;
-}
-
-export default function OmniContainer({ logEvent }: Props) {
+export default function OmniContainer() {
   const showOmni = useRef<boolean>();
+  const modalRef = useRef<HTMLDivElement>(null);
   const [showModal, setShowModal] = useState(false);
+  const handleLogEvent = (show: boolean) =>
+    logEvent(LOG_ACTIONS_OMNIBAR_TRIGGERED, {
+      show_omni: show,
+    });
+  const handleClose = () => {
+    showOmni.current = false;
+    setShowModal(false);
+    handleLogEvent(false);
+  };
 
   useComponentDidMount(() => {
     showOmni.current = false;
+
     function handleKeydown(event: KeyboardEvent) {
       if (!isFeatureEnabled(FeatureFlag.OMNIBAR)) return;
       const controlOrCommand = event.ctrlKey || event.metaKey;
       const isOk = ['KeyK'].includes(event.code);
       const isEsc = event.key === 'Escape';
+
       if (isEsc && showOmni.current) {
-        logEvent(LOG_ACTIONS_OMNIBAR_TRIGGERED, {
-          show_omni: false,
-        });
-        showOmni.current = false;
-        setShowModal(false);
+        handleClose();
         return;
       }
       if (controlOrCommand && isOk) {
-        logEvent(LOG_ACTIONS_OMNIBAR_TRIGGERED, {
-          show_omni: !!showOmni.current,
-        });
         showOmni.current = !showOmni.current;
         setShowModal(showOmni.current);
-        if (showOmni.current) {
-          document.getElementById('InputOmnibar')?.focus();
-        }
+        handleLogEvent(!!showOmni.current);
+      }
+    }
+
+    function handleClickOutside(event: MouseEvent) {
+      if (
+        modalRef.current &&
+        !modalRef.current.contains(event.target as Node)
+      ) {
+        handleClose();
       }
     }
 
+    document.addEventListener('mousedown', handleClickOutside);
     document.addEventListener('keydown', handleKeydown);
-    return () => document.removeEventListener('keydown', handleKeydown);
+    return () => {
+      document.removeEventListener('keydown', handleKeydown);
+      document.removeEventListener('mousedown', handleClickOutside);
+    };
   });
 
   return (
@@ -81,12 +94,15 @@ export default function OmniContainer({ logEvent }: Props) {
       hideFooter
       closable={false}
       onHide={() => {}}
+      destroyOnClose
     >
-      <Omnibar
-        id="InputOmnibar"
-        placeholder="Search all dashboards"
-        extensions={[getDashboards]}
-      />
+      <div ref={modalRef}>
+        <Omnibar
+          id="InputOmnibar"
+          placeholder={t('Search all dashboards')}
+          extensions={[getDashboards]}
+        />
+      </div>
     </OmniModal>
   );
 }
diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx
@@ -289,7 +289,7 @@ class Dashboard extends React.PureComponent {
     }
     return (
       <>
-        <OmniContainer logEvent={this.props.actions.logEvent} />
+        <OmniContainer />
         <DashboardBuilder />
       </>
     );

diff --git a/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx b/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx
@@ -46,6 +46,7 @@ import FaveStar from 'src/components/FaveStar';
 import PropertiesModal from 'src/dashboard/components/PropertiesModal';
 import { Tooltip } from 'src/components/Tooltip';
 import ImportModelsModal from 'src/components/ImportModal/index';
+import OmniContainer from 'src/components/OmniContainer';
 
 import Dashboard from 'src/dashboard/containers/Dashboard';
 import DashboardCard from './DashboardCard';
@@ -642,6 +643,9 @@ function DashboardList(props: DashboardListProps) {
         passwordFields={passwordFields}
         setPasswordFields={setPasswordFields}
       />
+
+      <OmniContainer />
+
       {preparingExport && <Loading />}
     </>
   );