Skip to content

Commit

Permalink
feat: Explore popovers should close on escape (apache#19902)
Browse files Browse the repository at this point in the history 
* feat: Explore popovers should close on escape

* review comments
  • Loading branch information
@diegomedina248 @philipher29
diegomedina248 authored and philipher29 committed Jun 9, 2022
1 parent 5004335 commit 8d3ac52
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 23 deletions.
83 changes: 69 additions & 14 deletions superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/
import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import { render, screen, fireEvent } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import { waitFor } from '@testing-library/react';

Expand All @@ -29,6 +29,16 @@ const createProps = (): Partial<PopoverProps> => ({
content: <span data-test="control-popover-content">Information</span>,
});

const TestComponent: React.FC<PopoverProps> = props => (
<div id="controlSections">
<div data-test="outer-container">
<ControlPopover {...props}>
<span data-test="control-popover">Click me</span>
</ControlPopover>
</div>
</div>
);

const setupTest = (props: Partial<PopoverProps> = createProps()) => {
const setStateMock = jest.fn();
jest
Expand All @@ -38,19 +48,12 @@ const setupTest = (props: Partial<PopoverProps> = createProps()) => {
state === 'right' ? setStateMock : jest.fn(),
]) as any);

const { container } = render(
<div data-test="outer-container">
<div id="controlSections">
<ControlPopover {...props}>
<span data-test="control-popover">Click me</span>
</ControlPopover>
</div>
</div>,
);
const { container, rerender } = render(<TestComponent {...props} />);

return {
props,
container,
rerender,
setStateMock,
};
};
Expand All @@ -67,7 +70,7 @@ test('Should render', () => {
expect(screen.getByTestId('control-popover-content')).toBeInTheDocument();
});

test('Should lock the vertical scroll when the popup is visible', () => {
test('Should lock the vertical scroll when the popover is visible', () => {
setupTest();
expect(screen.getByTestId('control-popover')).toBeInTheDocument();
expect(screen.getByTestId('outer-container')).not.toHaveStyle(
Expand All @@ -83,7 +86,7 @@ test('Should lock the vertical scroll when the popup is visible', () => {
);
});

test('Should place popup at the top', async () => {
test('Should place popover at the top', async () => {
const { setStateMock } = setupTest({
...createProps(),
getVisibilityRatio: () => 0.2,
Expand All @@ -97,7 +100,7 @@ test('Should place popup at the top', async () => {
});
});

test('Should place popup at the center', async () => {
test('Should place popover at the center', async () => {
const { setStateMock } = setupTest({
...createProps(),
getVisibilityRatio: () => 0.5,
Expand All @@ -111,7 +114,7 @@ test('Should place popup at the center', async () => {
});
});

test('Should place popup at the bottom', async () => {
test('Should place popover at the bottom', async () => {
const { setStateMock } = setupTest({
...createProps(),
getVisibilityRatio: () => 0.7,
Expand All @@ -124,3 +127,55 @@ test('Should place popup at the bottom', async () => {
expect(setStateMock).toHaveBeenCalledWith('rightBottom');
});
});

test('Should close popover on escape press', async () => {
setupTest({
...createProps(),
destroyTooltipOnHide: true,
});

expect(screen.getByTestId('control-popover')).toBeInTheDocument();
expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();
userEvent.click(screen.getByTestId('control-popover'));
expect(await screen.findByText('Control Popover Test')).toBeInTheDocument();

// Ensure that pressing any other key than escape does nothing
fireEvent.keyDown(screen.getByTestId('control-popover'), {
key: 'Enter',
code: 13,
charCode: 13,
});
expect(await screen.findByText('Control Popover Test')).toBeInTheDocument();

// Escape should close the popover
fireEvent.keyDown(screen.getByTestId('control-popover'), {
key: 'Escape',
code: 27,
charCode: 0,
});

await waitFor(() => {
expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();
});
});

test('Controlled mode', async () => {
const baseProps = {
...createProps(),
destroyTooltipOnHide: true,
visible: false,
};

const { rerender } = setupTest(baseProps);

expect(screen.getByTestId('control-popover')).toBeInTheDocument();
expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();

rerender(<TestComponent {...baseProps} visible />);
expect(await screen.findByText('Control Popover Test')).toBeInTheDocument();

rerender(<TestComponent {...baseProps} />);
await waitFor(() => {
expect(screen.queryByText('Control Popover Test')).not.toBeInTheDocument();
});
});
55 changes: 46 additions & 9 deletions superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useCallback, useEffect, useRef } from 'react';
import React, { useCallback, useRef, useEffect, useState } from 'react';

import Popover, {
PopoverProps as BasePopoverProps,
Expand All @@ -25,7 +25,7 @@ import Popover, {

const sectionContainerId = 'controlSections';
const getSectionContainerElement = () =>
document.getElementById(sectionContainerId)?.parentElement;
document.getElementById(sectionContainerId)?.lastElementChild as HTMLElement;

const getElementYVisibilityRatioOnContainer = (node: HTMLElement) => {
const containerHeight = window?.innerHeight;
Expand All @@ -44,9 +44,14 @@ export type PopoverProps = BasePopoverProps & {
const ControlPopover: React.FC<PopoverProps> = ({
getPopupContainer,
getVisibilityRatio = getElementYVisibilityRatioOnContainer,
visible: visibleProp,
...props
}) => {
const triggerElementRef = useRef<HTMLElement>();

const [visible, setVisible] = useState(
visibleProp === undefined ? props.defaultVisible : visibleProp,
);
const [placement, setPlacement] = React.useState<TooltipPlacement>('right');

const calculatePlacement = useCallback(() => {
Expand All @@ -69,7 +74,11 @@ const ControlPopover: React.FC<PopoverProps> = ({

const element = getSectionContainerElement();
if (element) {
element.style.overflowY = visible ? 'hidden' : 'auto';
element.style.setProperty(
'overflow-y',
visible ? 'hidden' : 'auto',
'important',
);
}
},
[calculatePlacement],
Expand All @@ -88,25 +97,53 @@ const ControlPopover: React.FC<PopoverProps> = ({
);

const handleOnVisibleChange = useCallback(
(visible: boolean) => {
if (props.visible === undefined) {
(visible: boolean | undefined) => {
if (visible === undefined) {
changeContainerScrollStatus(visible);
}

props.onVisibleChange?.(visible);
setVisible(!!visible);
props.onVisibleChange?.(!!visible);
},
[props, changeContainerScrollStatus],
);

const handleDocumentKeyDownListener = useCallback(
(event: KeyboardEvent) => {
if (event.key === 'Escape') {
setVisible(false);
props.onVisibleChange?.(false);
}
},
[props],
);

useEffect(() => {
if (props.visible !== undefined) {
changeContainerScrollStatus(props.visible);
if (visibleProp !== undefined) {
setVisible(!!visibleProp);
}
}, [props.visible, changeContainerScrollStatus]);
}, [visibleProp]);

useEffect(() => {
if (visible !== undefined) {
changeContainerScrollStatus(visible);
}
}, [visible, changeContainerScrollStatus]);

useEffect(() => {
if (visible) {
document.addEventListener('keydown', handleDocumentKeyDownListener);
}

return () => {
document.removeEventListener('keydown', handleDocumentKeyDownListener);
};
}, [handleDocumentKeyDownListener, visible]);

return (
<Popover
{...props}
visible={visible}
arrowPointAtCenter
placement={placement}
onVisibleChange={handleOnVisibleChange}
Expand Down

0 comments on commit 8d3ac52

Please sign in to comment.