You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2022/05/13 18:28:53 UTC
[superset] branch master updated: feat: Explore popovers should close on escape (#19902)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new dbc653d442 feat: Explore popovers should close on escape (#19902)
dbc653d442 is described below
commit dbc653d442e48078546210a76df5eb216b30b707
Author: Diego Medina <di...@gmail.com>
AuthorDate: Fri May 13 14:28:39 2022 -0400
feat: Explore popovers should close on escape (#19902)
* feat: Explore popovers should close on escape
* review comments
---
.../ControlPopover/ControlPopover.test.tsx | 83 ++++++++++++++++++----
.../controls/ControlPopover/ControlPopover.tsx | 55 +++++++++++---
2 files changed, 115 insertions(+), 23 deletions(-)
diff --git a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx
index de14f78f75..3da003be4a 100644
--- a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx
+++ b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.test.tsx
@@ -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';
@@ -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
@@ -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,
};
};
@@ -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(
@@ -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,
@@ -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,
@@ -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,
@@ -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();
+ });
+});
diff --git a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
index 70ade69f27..f84194c43c 100644
--- a/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
+++ b/superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { useCallback, useRef, useEffect } from 'react';
+import React, { useCallback, useRef, useEffect, useState } from 'react';
import Popover, {
PopoverProps as BasePopoverProps,
@@ -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;
@@ -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(() => {
@@ -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],
@@ -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}