From c14364c616e709b16774072504657caa857045dc Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Tue, 24 Aug 2021 11:52:07 +0300 Subject: [PATCH] chore: Enhance Omnibar (#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 * Enhance * Handle close * Localize placeholder * Fix tests * Clear input on close * Destroy modal on close * Clean up * Fix tests Co-authored-by: Evan Rusackas --- .../src/components/Modal/Modal.tsx | 1 + .../OmniContainer/OmniContainer.test.tsx | 18 +++--- .../src/components/OmniContainer/Omnibar.tsx | 3 +- .../src/components/OmniContainer/index.tsx | 62 ++++++++++++------- .../src/dashboard/components/Dashboard.jsx | 2 +- .../views/CRUD/dashboard/DashboardList.tsx | 4 ++ 6 files changed, 54 insertions(+), 36 deletions(-) diff --git a/superset-frontend/src/components/Modal/Modal.tsx b/superset-frontend/src/components/Modal/Modal.tsx index a23f40a536..3dbdb62a3c 100644 --- 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 index 61dd7be394..d797727ed2 100644 --- 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(
- +
, ); @@ -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(
- +
, ); @@ -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(
- +
, ); @@ -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(
- +
, ); @@ -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 index 2cba1c99a5..aeeffd1ef6 100644 --- 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 index 5a18d5e4e4..cd56cc0462 100644 --- 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(); + const modalRef = useRef(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 > - +
+ +
); } diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx index 21bd809e13..e168b394a5 100644 --- 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 ( <> - + ); diff --git a/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx b/superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx index 9c2bd841c0..efb1544045 100644 --- 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} /> + + + {preparingExport && } );