Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Enhance Omnibar #16273

Merged
merged 18 commits into from
Aug 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset-frontend/src/components/Modal/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export interface ModalProps {
wrapProps?: object;
height?: string;
closable?: boolean;
destroyOnClose?: boolean;
}

interface StyledModalProps {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
);

Expand All @@ -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>,
);

Expand All @@ -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>,
);

Expand All @@ -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>,
);

Expand All @@ -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();
});
3 changes: 2 additions & 1 deletion superset-frontend/src/components/OmniContainer/Omnibar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
/>
);
}
62 changes: 39 additions & 23 deletions superset-frontend/src/components/OmniContainer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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 (
Expand All @@ -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>
);
}
2 changes: 1 addition & 1 deletion superset-frontend/src/dashboard/components/Dashboard.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ class Dashboard extends React.PureComponent {
}
return (
<>
<OmniContainer logEvent={this.props.actions.logEvent} />
<OmniContainer />
<DashboardBuilder />
</>
);
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -642,6 +643,9 @@ function DashboardList(props: DashboardListProps) {
passwordFields={passwordFields}
setPasswordFields={setPasswordFields}
/>

<OmniContainer />

{preparingExport && <Loading />}
</>
);
Expand Down