Skip to content

Commit

Permalink
refactor(Popover): Upgrade Popover to Antd5 (apache#31973)
Browse files Browse the repository at this point in the history
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
  • Loading branch information
alexandrusoare and geido authored Feb 10, 2025
1 parent 06f8f8e commit 0030f46
Show file tree
Hide file tree
Showing 30 changed files with 157 additions and 228 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('Horizontal FilterBar', () => {
cy.getBySel('filter-control-name')
.contains('test_12')
.should('not.be.visible');
cy.get('.ant-popover-inner-content').scrollTo('bottom');
cy.get('.antd5-popover-inner').scrollTo('bottom');
cy.getBySel('filter-control-name').contains('test_12').should('be.visible');
});

Expand Down Expand Up @@ -226,7 +226,7 @@ describe('Horizontal FilterBar', () => {
cy.getBySel('slice-header').within(() => {
cy.get('.filter-counts').trigger('mouseover');
});
cy.get('.filterStatusPopover').contains('test_9').click();
cy.getBySel('filter-status-popover').contains('test_9').click();
cy.getBySel('dropdown-content').should('be.visible');
cy.get('.ant-select-focused').should('be.visible');
});
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,19 +456,19 @@ export function applyAdvancedTimeRangeFilterOnDashboard(
endRange?: string,
) {
cy.get('.control-label').contains('RANGE TYPE').should('be.visible');
cy.get('.ant-popover-content .ant-select-selector')
cy.get('.antd5-popover-content .ant-select-selector')
.should('be.visible')
.click();
cy.get(`[label="Advanced"]`).should('be.visible').click();
cy.get('.section-title').contains('Advanced Time Range').should('be.visible');
if (startRange) {
cy.get('.ant-popover-inner-content')
cy.get('.antd5-popover-inner-content')
.find('[class^=ant-input]')
.first()
.type(`${startRange}`);
}
if (endRange) {
cy.get('.ant-popover-inner-content')
cy.get('.antd5-popover-inner-content')
.find('[class^=ant-input]')
.last()
.type(`${endRange}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ export const exploreView = {
timeSection: {
timeRangeFilter: dataTestLocator('time-range-trigger'),
timeRangeFilterModal: {
container: '.ant-popover-content',
container: '.antd5-popover-content',
footer: '.footer',
cancelButton: dataTestLocator('cancel-button'),
configureLastTimeRange: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
* under the License.
*/
import { useEffect, useState } from 'react';
import { Popover } from 'antd';
import { Popover } from 'antd-v5';
import type ReactAce from 'react-ace';
import type { PopoverProps } from 'antd/lib/popover';
import type { PopoverProps } from 'antd-v5/lib/popover';
import { CalculatorOutlined } from '@ant-design/icons';
import { css, styled, useTheme, t } from '@superset-ui/core';

Expand Down Expand Up @@ -72,7 +72,7 @@ export const SQLPopover = (props: PopoverProps & { sqlExpression: string }) => {
/>
}
placement="bottomLeft"
arrowPointAtCenter
arrow={{ pointAtCenter: true }}
title={t('SQL expression')}
{...props}
>
Expand Down
8 changes: 6 additions & 2 deletions superset-frontend/src/GlobalStyles.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,18 @@ export const GlobalStyles = () => (
.echarts-tooltip[style*='visibility: hidden'] {
display: none !important;
}
// Ant Design is applying inline z-index styles causing troubles
// TODO: Remove z-indexes when Ant Design is fully upgraded to v5
// Prefer vanilla Ant Design z-indexes that should work out of the box
.ant-popover,
.antd5-dropdown,
.ant-dropdown,
.ant-select-dropdown,
.antd5-modal-wrap,
.antd5-modal-mask,
.antd5-picker-dropdown {
.antd5-picker-dropdown,
.ant-popover,
.antd5-popover {
z-index: ${theme.zIndex.max} !important;
}
Expand Down
20 changes: 11 additions & 9 deletions superset-frontend/src/components/DropdownContainer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ const DropdownContainer = forwardRef(
const { current } = ref;
const [itemsWidth, setItemsWidth] = useState<number[]>([]);
const [popoverVisible, setPopoverVisible] = useState(false);

// We use React.useState to be able to mock the state in Jest
const [overflowingIndex, setOverflowingIndex] = useState<number>(-1);

Expand Down Expand Up @@ -181,11 +180,13 @@ const DropdownContainer = forwardRef(
);

useLayoutEffect(() => {
if (popoverVisible) {
return;
}
const container = current?.children.item(0);
if (container) {
const { children } = container;
const childrenArray = Array.from(children);

// If items length change, add all items to the container
// and recalculate the widths
if (itemsWidth.length !== items.length) {
Expand Down Expand Up @@ -341,11 +342,7 @@ const DropdownContainer = forwardRef(
<>
<Global
styles={css`
.ant-popover-inner-content {
max-height: ${MAX_HEIGHT}px;
overflow: ${showOverflow ? 'auto' : 'visible'};
padding: ${theme.gridUnit * 3}px ${theme.gridUnit * 4}px;
.antd5-popover-inner {
// Some OS versions only show the scroll when hovering.
// These settings will make the scroll always visible.
::-webkit-scrollbar {
Expand All @@ -365,11 +362,16 @@ const DropdownContainer = forwardRef(
}
`}
/>

<Popover
overlayInnerStyle={{
maxHeight: `${MAX_HEIGHT}px`,
overflow: showOverflow ? 'auto' : 'visible',
}}
content={popoverContent}
trigger="click"
visible={popoverVisible}
onVisibleChange={visible => setPopoverVisible(visible)}
open={popoverVisible}
onOpenChange={visible => setPopoverVisible(visible)}
placement="bottom"
forceRender={forceRender}
>
Expand Down
15 changes: 13 additions & 2 deletions superset-frontend/src/components/Popover/Popover.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
* under the License.
*/
import Button from 'src/components/Button';
import { PopoverProps } from 'antd/lib/popover';
import Popover from '.';
import Popover, { PopoverProps } from 'src/components/Popover';

export default {
title: 'Popover',
Expand Down Expand Up @@ -66,6 +65,8 @@ const TRIGGERS = {
InteractivePopover.args = {
content: 'Popover sample content',
title: 'Popover title',
arrow: true,
color: '#fff',
};

InteractivePopover.argTypes = {
Expand All @@ -79,4 +80,14 @@ InteractivePopover.argTypes = {
control: { type: 'select' },
options: TRIGGERS.options,
},
arrow: {
name: 'arrow',
control: { type: 'boolean' },
description: "Change arrow's visible state",
},
color: {
name: 'color',
control: { type: 'color' },
description: 'The background color of the popover.',
},
};
14 changes: 7 additions & 7 deletions superset-frontend/src/components/Popover/Popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,20 @@ import userEvent from '@testing-library/user-event';
import { supersetTheme } from '@superset-ui/core';
import Icons from 'src/components/Icons';
import Button from 'src/components/Button';
import Popover from '.';
import Popover from 'src/components/Popover';

test('should render', () => {
const { container } = render(<Popover />);
expect(container).toBeInTheDocument();
});

test('should render a title when visible', () => {
render(<Popover title="Popover title" visible />);
render(<Popover title="Popover title" open />);
expect(screen.getByText('Popover title')).toBeInTheDocument();
});

test('should render some content when visible', () => {
render(<Popover content="Content sample" visible />);
render(<Popover content="Content sample" open />);
expect(screen.getByText('Content sample')).toBeInTheDocument();
});

Expand All @@ -61,22 +61,22 @@ test('renders with icon child', async () => {
});

test('fires an event when visibility is changed', async () => {
const onVisibleChange = jest.fn();
const onOpenChange = jest.fn();
render(
<Popover
content="Content sample"
title="Popover title"
onVisibleChange={onVisibleChange}
onOpenChange={onOpenChange}
>
<Button>Hover me</Button>
</Popover>,
);
userEvent.hover(screen.getByRole('button'));
await waitFor(() => expect(onVisibleChange).toHaveBeenCalledTimes(1));
await waitFor(() => expect(onOpenChange).toHaveBeenCalledTimes(1));
});

test('renders with theme', () => {
render(<Popover content="Content sample" title="Popover title" visible />);
render(<Popover content="Content sample" title="Popover title" open />);
const title = screen.getByText('Popover title');
expect(title).toHaveStyle({
fontSize: supersetTheme.gridUnit * 3.5,
Expand Down
27 changes: 0 additions & 27 deletions superset-frontend/src/components/Popover/Popover.tsx

This file was deleted.

14 changes: 9 additions & 5 deletions superset-frontend/src/components/Popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/
export type { PopoverProps } from 'antd/lib/popover';
export type { TooltipPlacement } from 'antd/lib/tooltip';
import { Popover as AntdPopover } from 'antd-v5';
import { PopoverProps as AntdPopoverProps } from 'antd-v5/lib/popover';

// Eventually Popover can be wrapped and customized in this file
// for now we're just redirecting
export { Popover as default } from './Popover';
export interface PopoverProps extends AntdPopoverProps {
forceRender?: boolean;
}

const Popover = (props: PopoverProps) => <AntdPopover {...props} />;

export default Popover;
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function HeaderWithRadioGroup(props: HeaderWithRadioGroupProps) {
>
<Popover
trigger="click"
visible={popoverVisible}
open={popoverVisible}
content={
<div>
<div
Expand Down Expand Up @@ -72,7 +72,7 @@ function HeaderWithRadioGroup(props: HeaderWithRadioGroupProps) {
</div>
}
placement="bottomLeft"
arrowPointAtCenter
arrow={{ pointAtCenter: true }}
>
<Icons.SettingOutlined
iconSize="m"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import {
useTheme,
useElementOnScreen,
} from '@superset-ui/core';
import { Global } from '@emotion/react';
import { useDispatch, useSelector } from 'react-redux';
import ErrorBoundary from 'src/components/ErrorBoundary';
import BuilderComponentPane from 'src/dashboard/components/BuilderComponentPane';
Expand Down Expand Up @@ -653,13 +652,6 @@ const DashboardBuilder = () => {
</Droppable>
</StyledHeader>
<StyledContent fullSizeChartId={fullSizeChartId}>
<Global
styles={css`
// @z-index-above-dashboard-header (100) + 1 = 101
${fullSizeChartId &&
`div > .filterStatusPopover.ant-popover{z-index: 101}`}
`}
/>
{!editMode &&
!topLevelTabs &&
dashboardLayout[DASHBOARD_GRID_ID]?.children?.length === 0 && (
Expand Down
Loading

0 comments on commit 0030f46

Please sign in to comment.