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.

Sign up for GitHub

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

refactor(Popover): Upgrade Popover to Antd5 #31973

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
+106 −202
Open

refactor(Popover): Upgrade Popover to Antd5 #31973

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
be1d564
refactor(Popover): upgrade Popover to antd5
alexandrusoare Jan 21, 2025
f88fa0e
refactor(DetailsPanel): removing custom styles
alexandrusoare Jan 21, 2025
e6cc1fe
refactor(components): upgrading to andt5
alexandrusoare Jan 21, 2025
636c1a1
refactor(popover): updating to Antd5
alexandrusoare Jan 21, 2025
3c99830
refactor(popover): updating to Antd5
alexandrusoare Jan 21, 2025
a69a8b0
refactor(Popover): upgrading to antd5
alexandrusoare Jan 22, 2025
584239b
refactor(Popover): upgrading to antd5
alexandrusoare Jan 22, 2025
2839455
Merge branch 'master' into alexandrusoare/refactor/antd5-popover
alexandrusoare Jan 24, 2025
74d9a30
fix(e2e): fixed e2e test
alexandrusoare Jan 24, 2025
40a2034
fix(e2e): fixed e2e test
alexandrusoare Jan 24, 2025
cabf53f
feat(story): added props to storybook
alexandrusoare Jan 24, 2025
159a7fb
feat(styles): removed some styles
alexandrusoare Jan 24, 2025
4677efa
feat(style): removed some styles
alexandrusoare Jan 24, 2025
18ef69e
feat(style): removed some styles
alexandrusoare Jan 24, 2025
ad18747
refactor(Popover): small changes from PR reviews
alexandrusoare Jan 28, 2025
e70f6a4
fix(bug): fixed bug related to dropdowncontainer
alexandrusoare Jan 29, 2025
491a643
chore(clean): small cleaning up
alexandrusoare Jan 29, 2025
67a7571
refactor(pr): small refactor from pr reviews
alexandrusoare Jan 31, 2025
79988e4
refactor(dropdowncontainer): small changes for PR reviews
alexandrusoare Feb 4, 2025
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
4 changes: 2 additions & 2 deletions superset-frontend/cypress-base/cypress/e2e/dashboard/horizontalFilterBar.test.ts
View file
Open in desktop
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.get('[data-test="filter-status-popover"]').contains('test_9').click();
alexandrusoare marked this conversation as resolved.
Show resolved Hide resolved
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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion superset-frontend/cypress-base/cypress/support/directories.ts
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion superset-frontend/cypress-base/cypress/support/e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ Cypress.Commands.add('login', () => {
}).then(response => {
if (response.status === 302) {
// If there's a redirect, follow it manually
const redirectUrl = response.headers['location'];
const redirectUrl = response.headers.location;
alexandrusoare marked this conversation as resolved.
Show resolved Hide resolved
cy.request({
method: 'GET',
url: redirectUrl,
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/packages/superset-ui-chart-controls/src/components/SQLPopover.tsx
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion superset-frontend/src/GlobalStyles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ 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,
Expand Down
15 changes: 8 additions & 7 deletions superset-frontend/src/components/DropdownContainer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,11 +341,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-content {
// Some OS versions only show the scroll when hovering.
// These settings will make the scroll always visible.
::-webkit-scrollbar {
Expand All @@ -366,10 +362,15 @@ const DropdownContainer = forwardRef(
`}
/>
<Popover
overlayInnerStyle={{
maxHeight: `${MAX_HEIGHT}px`,
overflow: showOverflow ? 'auto' : 'visible',
padding: `${theme.gridUnit * 3}px ${theme.gridUnit * 4}px`,
}}
geido marked this conversation as resolved.
Show resolved Hide resolved
content={popoverContent}
trigger="click"
visible={popoverVisible}
onVisibleChange={visible => setPopoverVisible(visible)}
open={popoverVisible}
onOpenChange={visible => setPopoverVisible(visible)}
placement="bottom"
forceRender={forceRender}
>
Expand Down
21 changes: 19 additions & 2 deletions superset-frontend/src/components/Popover/Popover.stories.tsx
View file
Open in desktop
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,9 @@ const TRIGGERS = {
InteractivePopover.args = {
content: 'Popover sample content',
title: 'Popover title',
arrow: true,
open: false,
color: '#fff',
};

InteractivePopover.argTypes = {
Expand All @@ -79,4 +81,19 @@ 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.',
},
open: {
name: 'open',
control: { type: 'boolean' },
description: 'Whether the floating tooltip card is open or not.',
},
};
14 changes: 7 additions & 7 deletions superset-frontend/src/components/Popover/Popover.test.tsx
View file
Open in desktop
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
View file
Open in desktop

This file was deleted.

14 changes: 9 additions & 5 deletions superset-frontend/src/components/Popover/index.tsx
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for? I don't see forceRender exposed in the Popover props of Ant Design. Will this have any effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was being used before, I just combined the Popover.tsx file with the index.tsx file -> https://github.com/apache/superset/pull/31973/files/18ef69e7e88cc0389c4dd991050e176988acb9e8#diff-575bb0af856e1f113959bcf0ec737a412cb23b3b5ed154012d993d5be95da187
And the forceRender prop is being used in some places, here's 1 for example: superset/superset-frontend/src/components/DropdownContainer/index.tsx

Copy link
Member

@geido geido Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this answers my question. If forceRender is not an option that Popover accepts in Ant Design, what is the use of it? Is this actually doing anything? Can you confirm that, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The forceRender option ensures that the Popover’s content is rendered in the DOM even when the Popover is not opened. I verified that it behaves as expected, which confirms that it works even though it isn’t an official prop in Ant Design.

}

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

export default Popover;
4 changes: 2 additions & 2 deletions superset-frontend/src/components/Table/header-renderers/HeaderWithRadioGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function HeaderWithRadioGroup(props: HeaderWithRadioGroupProps) {
>
<Popover
trigger="click"
visible={popoverVisible}
open={popoverVisible}
content={
<div>
<div
Expand Down Expand Up @@ -74,7 +74,7 @@ function HeaderWithRadioGroup(props: HeaderWithRadioGroupProps) {
</div>
}
placement="bottomLeft"
arrowPointAtCenter
arrow={{ pointAtCenter: true }}
>
<Icons.SettingOutlined
iconSize="m"
Expand Down
5 changes: 2 additions & 3 deletions superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
View file
Open in desktop
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 @@ -657,13 +656,13 @@ const DashboardBuilder = () => {
</Droppable>
</StyledHeader>
<StyledContent fullSizeChartId={fullSizeChartId}>
<Global
{/* <Global
geido marked this conversation as resolved.
Show resolved Hide resolved
styles={css`
// @z-index-above-dashboard-header (100) + 1 = 101
${fullSizeChartId &&
`div > .filterStatusPopover.ant-popover{z-index: 101}`}
`}
/>
/> */}
alexandrusoare marked this conversation as resolved.
Show resolved Hide resolved
{!editMode &&
!topLevelTabs &&
dashboardLayout[DASHBOARD_GRID_ID]?.children?.length === 0 && (
Expand Down
60 changes: 6 additions & 54 deletions superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
import { RefObject, useEffect, useRef, KeyboardEvent } from 'react';

import { useSelector } from 'react-redux';
import { Global, css } from '@emotion/react';
import { t } from '@superset-ui/core';
import { t, useTheme } from '@superset-ui/core';
import Popover from 'src/components/Popover';
import {
FiltersContainer,
Expand Down Expand Up @@ -120,7 +119,7 @@ const DetailsPanelPopover = ({

const indicatorKey = (indicator: Indicator): string =>
`${indicator.column} - ${indicator.name}`;

const theme = useTheme();
const content = (
<FiltersDetailsContainer
ref={popoverContentRef}
Expand All @@ -129,54 +128,6 @@ const DetailsPanelPopover = ({
onKeyDown={handleKeyDown}
role="menu"
>
<Global
styles={theme => css`
.filterStatusPopover {
.ant-popover-inner {
background-color: ${theme.colors.grayscale.dark2}cc;
.ant-popover-inner-content {
padding: ${theme.gridUnit * 2}px;
}
}
&.ant-popover-placement-bottom,
&.ant-popover-placement-bottomLeft,
&.ant-popover-placement-bottomRight {
& > .ant-popover-content > .ant-popover-arrow {
border-top-color: ${theme.colors.grayscale.dark2}cc;
border-left-color: ${theme.colors.grayscale.dark2}cc;
}
}
&.ant-popover-placement-top,
&.ant-popover-placement-topLeft,
&.ant-popover-placement-topRight {
& > .ant-popover-content > .ant-popover-arrow {
border-bottom-color: ${theme.colors.grayscale.dark2}cc;
border-right-color: ${theme.colors.grayscale.dark2}cc;
}
}
&.ant-popover-placement-left,
&.ant-popover-placement-leftTop,
&.ant-popover-placement-leftBottom {
& > .ant-popover-content > .ant-popover-arrow {
border-top-color: ${theme.colors.grayscale.dark2}cc;
border-right-color: ${theme.colors.grayscale.dark2}cc;
}
}
&.ant-popover-placement-right,
&.ant-popover-placement-rightTop,
&.ant-popover-placement-rightBottom {
& > .ant-popover-content > .ant-popover-arrow {
border-bottom-color: ${theme.colors.grayscale.dark2}cc;
border-left-color: ${theme.colors.grayscale.dark2}cc;
}
}
&.ant-popover {
color: ${theme.colors.grayscale.light4};
z-index: 99;
}
}
`}
/>
<div>
{appliedCrossFilterIndicators.length ? (
<div>
Expand Down Expand Up @@ -224,12 +175,13 @@ const DetailsPanelPopover = ({

return (
<Popover
overlayClassName="filterStatusPopover"
color={`${theme.colors.grayscale.dark2}cc`}
alexandrusoare marked this conversation as resolved.
Show resolved Hide resolved
content={content}
visible={popoverVisible}
onVisibleChange={handleVisibility}
open={popoverVisible}
onOpenChange={handleVisibility}
placement="bottomRight"
trigger={['hover']}
data-test="filter-status-popover"
>
{children}
</Popover>
Expand Down
Loading
Loading