Skip to content

Commit

Permalink
fix: Popovers in Explore not attached to the fields they are triggere…
Browse files Browse the repository at this point in the history
…d by (apache#19139)

* fix: Popovers in Explore not attached to the fields they are triggered by

* fix

* PR comment

* remove unused import
  • Loading branch information
diegomedina248 authored Mar 17, 2022
1 parent 3b427b2 commit 0277ebc
Show file tree
Hide file tree
Showing 14 changed files with 277 additions and 35 deletions.
3 changes: 3 additions & 0 deletions superset-frontend/src/components/Popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
*/
import { Popover } from 'antd';

export { PopoverProps } from 'antd/lib/popover';
export { TooltipPlacement } from 'antd/lib/tooltip';

// Eventually Popover can be wrapped and customized in this file
// for now we're just redirecting
export default Popover;
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ import { List } from 'src/components';
import { connect } from 'react-redux';
import { t, withTheme } from '@superset-ui/core';
import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
import Popover from 'src/components/Popover';
import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
import { getChartKey } from 'src/explore/exploreUtils';
import { runAnnotationQuery } from 'src/components/Chart/chartAction';
import CustomListItem from 'src/explore/components/controls/CustomListItem';
import ControlPopover from '../ControlPopover/ControlPopover';

const AnnotationLayer = AsyncEsmComponent(
() => import('./AnnotationLayer'),
Expand Down Expand Up @@ -167,10 +167,9 @@ class AnnotationLayerControl extends React.PureComponent {
const addedAnnotation = this.props.value[addedAnnotationIndex];

const annotations = this.props.value.map((anno, i) => (
<Popover
<ControlPopover
key={i}
trigger="click"
placement="right"
title={t('Edit annotation layer')}
css={theme => ({
'&:hover': {
Expand All @@ -190,17 +189,16 @@ class AnnotationLayerControl extends React.PureComponent {
<span>{anno.name}</span>
<span style={{ float: 'right' }}>{this.renderInfo(anno)}</span>
</CustomListItem>
</Popover>
</ControlPopover>
));

const addLayerPopoverKey = 'add';
return (
<div>
<List bordered css={theme => ({ borderRadius: theme.gridUnit })}>
{annotations}
<Popover
<ControlPopover
trigger="click"
placement="right"
content={this.renderPopover(addLayerPopoverKey, addedAnnotation)}
title={t('Add annotation layer')}
visible={this.state.popoverVisible[addLayerPopoverKey]}
Expand All @@ -216,7 +214,7 @@ class AnnotationLayerControl extends React.PureComponent {
/>{' '}
&nbsp; {t('Add annotation layer')}
</CustomListItem>
</Popover>
</ControlPopover>
</List>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import { waitFor } from '@testing-library/react';

import ControlPopover, { PopoverProps } from './ControlPopover';

const createProps = (): Partial<PopoverProps> => ({
trigger: 'click',
title: 'Control Popover Test',
content: <span data-test="control-popover-content">Information</span>,
});

const setupTest = (props: Partial<PopoverProps> = createProps()) => {
const setStateMock = jest.fn();
jest
.spyOn(React, 'useState')
.mockImplementation(((state: any) => [
state,
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>,
);

return {
props,
container,
setStateMock,
};
};

afterEach(() => {
jest.restoreAllMocks();
});

test('Should render', () => {
setupTest();
expect(screen.getByTestId('control-popover')).toBeInTheDocument();
userEvent.click(screen.getByTestId('control-popover'));
expect(screen.getByText('Control Popover Test')).toBeInTheDocument();
expect(screen.getByTestId('control-popover-content')).toBeInTheDocument();
});

test('Should lock the vertical scroll when the popup is visible', () => {
setupTest();
expect(screen.getByTestId('control-popover')).toBeInTheDocument();
expect(screen.getByTestId('outer-container')).not.toHaveStyle(
'overflowY: hidden',
);
userEvent.click(screen.getByTestId('control-popover'));
expect(screen.getByTestId('outer-container')).toHaveStyle(
'overflowY: hidden',
);
userEvent.click(document.body);
expect(screen.getByTestId('outer-container')).not.toHaveStyle(
'overflowY: hidden',
);
});

test('Should place popup at the top', async () => {
const { setStateMock } = setupTest({
...createProps(),
getVisibilityRatio: () => 0.2,
});

expect(screen.getByTestId('control-popover')).toBeInTheDocument();
userEvent.click(screen.getByTestId('control-popover'));

await waitFor(() => {
expect(setStateMock).toHaveBeenCalledWith('rightTop');
});
});

test('Should place popup at the center', async () => {
const { setStateMock } = setupTest({
...createProps(),
getVisibilityRatio: () => 0.5,
});

expect(screen.getByTestId('control-popover')).toBeInTheDocument();
userEvent.click(screen.getByTestId('control-popover'));

await waitFor(() => {
expect(setStateMock).toHaveBeenCalledWith('right');
});
});

test('Should place popup at the bottom', async () => {
const { setStateMock } = setupTest({
...createProps(),
getVisibilityRatio: () => 0.7,
});

expect(screen.getByTestId('control-popover')).toBeInTheDocument();
userEvent.click(screen.getByTestId('control-popover'));

await waitFor(() => {
expect(setStateMock).toHaveBeenCalledWith('rightBottom');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import React, { useCallback, useRef, useEffect } from 'react';

import Popover, {
PopoverProps as BasePopoverProps,
TooltipPlacement,
} from 'src/components/Popover';

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

const getElementYVisibilityRatioOnContainer = (node: HTMLElement) => {
const containerHeight = window?.innerHeight;
const nodePositionInViewport = node.getBoundingClientRect()?.top;
if (!containerHeight || !nodePositionInViewport) {
return 0;
}

return nodePositionInViewport / containerHeight;
};

export type PopoverProps = BasePopoverProps & {
getVisibilityRatio?: typeof getElementYVisibilityRatioOnContainer;
};

const ControlPopover: React.FC<PopoverProps> = ({
getPopupContainer,
getVisibilityRatio = getElementYVisibilityRatioOnContainer,
...props
}) => {
const triggerElementRef = useRef<HTMLElement>();
const [placement, setPlacement] = React.useState<TooltipPlacement>('right');

const calculatePlacement = useCallback(() => {
const visibilityRatio = getVisibilityRatio(triggerElementRef.current!);

if (visibilityRatio < 0.35) {
setPlacement('rightTop');
} else if (visibilityRatio > 0.65) {
setPlacement('rightBottom');
} else {
setPlacement('right');
}
}, [getVisibilityRatio]);

const changeContainerScrollStatus = useCallback(
visible => {
if (triggerElementRef.current && visible) {
calculatePlacement();
}

const element = getSectionContainerElement();
if (element) {
element.style.overflowY = visible ? 'hidden' : 'auto';
}
},
[calculatePlacement],
);

const handleGetPopupContainer = useCallback(
(triggerNode: HTMLElement) => {
triggerElementRef.current = triggerNode;
setTimeout(() => {
calculatePlacement();
}, 0);

return getPopupContainer?.(triggerNode) || document.body;
},
[calculatePlacement, getPopupContainer],
);

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

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

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

return (
<Popover
{...props}
arrowPointAtCenter
placement={placement}
onVisibleChange={handleOnVisibleChange}
getPopupContainer={handleGetPopupContainer}
/>
);
};

export default ControlPopover;
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import { getClientErrorObject } from 'src/utils/getClientErrorObject';
import Button from 'src/components/Button';
import ControlHeader from 'src/explore/components/ControlHeader';
import Label, { Type } from 'src/components/Label';
import Popover from 'src/components/Popover';
import { Divider } from 'src/components';
import Icons from 'src/components/Icons';
import Select from 'src/components/Select/Select';
Expand All @@ -42,6 +41,7 @@ import { SLOW_DEBOUNCE } from 'src/constants';
import { testWithId } from 'src/utils/testUtils';
import { noOp } from 'src/utils/common';
import { FrameType } from './types';
import ControlPopover from '../ControlPopover/ControlPopover';

import {
CommonFrame,
Expand Down Expand Up @@ -86,7 +86,7 @@ const fetchTimeRange = async (timeRange: string) => {
}
};

const StyledPopover = styled(Popover)``;
const StyledPopover = styled(ControlPopover)``;
const StyledRangeType = styled(Select)`
width: 272px;
`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ import {
isAdhocColumn,
isColumnMeta,
} from '@superset-ui/chart-controls';
import Popover from 'src/components/Popover';
import { ExplorePopoverContent } from 'src/explore/components/ExploreContentPopover';
import ColumnSelectPopover from './ColumnSelectPopover';
import { DndColumnSelectPopoverTitle } from './DndColumnSelectPopoverTitle';
import ControlPopover from '../ControlPopover/ControlPopover';

interface ColumnSelectPopoverTriggerProps {
columns: ColumnMeta[];
Expand Down Expand Up @@ -137,8 +137,7 @@ const ColumnSelectPopoverTrigger = ({
);

return (
<Popover
placement="right"
<ControlPopover
trigger="click"
content={overlayContent}
defaultVisible={visible}
Expand All @@ -148,7 +147,7 @@ const ColumnSelectPopoverTrigger = ({
destroyTooltipOnHide
>
{children}
</Popover>
</ControlPopover>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ import React from 'react';
import sinon from 'sinon';
import { shallow } from 'enzyme';

import Popover from 'src/components/Popover';
import FilterBoxItemControl from 'src/explore/components/controls/FilterBoxItemControl';
import FormRow from 'src/components/FormRow';
import datasources from 'spec/fixtures/mockDatasource';
import ControlPopover from '../ControlPopover/ControlPopover';

const defaultProps = {
label: 'some label',
Expand All @@ -46,7 +46,7 @@ describe('FilterBoxItemControl', () => {
});

it('renders a Popover', () => {
expect(wrapper.find(Popover)).toExist();
expect(wrapper.find(ControlPopover)).toExist();
});

it('renderForms does the job', () => {
Expand Down
Loading

0 comments on commit 0277ebc

Please sign in to comment.