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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 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
dcad86c
fix(popover): fixed popover showing in the wrong place
alexandrusoare Feb 5, 2025
b9efb43
fix(popover): fix placement issue
alexandrusoare Feb 6, 2025
ff37da8
fix(unittest): fixed failing unittests
alexandrusoare Feb 7, 2025
701e4c2
refactor(controlPopover): small change
alexandrusoare Feb 7, 2025
a29d7e0
Merge branch 'master' of https://github.com/apache/superset into alex…
alexandrusoare Feb 7, 2025
5531d39
refactor(popover): set zIndex to 3000
alexandrusoare Feb 7, 2025
2327045
refactor(popover): set zIndex to 3000
alexandrusoare Feb 7, 2025
7725436
getting rid of console.log
alexandrusoare Feb 10, 2025
4614417
Merge branch 'master' into alexandrusoare/refactor/antd5-popover
geido Feb 10, 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
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
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) {
Copy link
Member

Choose a reason for hiding this comment

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

should we add it to the dependency array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think so, I added this statement because of a visual bug that was happening in certain cases. And I think that we don't need to recalculate the size when popover is closed .

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;
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

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;
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
Loading