refactor(Popover): Upgrade Popover to Antd5 #31973

alexandrusoare · 2025-01-23T13:58:23Z

SUMMARY

Upgrading Popover component to Antd5.

Fixes #31953

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

BEFORE

AFTER

BEFORE

AFTER

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

korbit-ai · 2025-01-23T13:58:27Z

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Unreliable Header Access ▹ view
	Redundant Z-Index Configuration ▹ view
	Filter popover z-index issue in full-size mode ▹ view

Files scanned

File Path	Reviewed
superset-frontend/src/components/Popover/index.tsx	✅
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/types.ts	✅
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopover.tsx	✅
superset-frontend/src/explore/components/controls/ContourControl/ContourPopoverTrigger.tsx	✅
superset-frontend/packages/superset-ui-chart-controls/src/components/SQLPopover.tsx	✅
superset-frontend/src/components/Popover/Popover.stories.tsx	✅
superset-frontend/src/dashboard/components/nativeFilters/FilterCard/index.tsx	✅
superset-frontend/src/components/Table/header-renderers/HeaderWithRadioGroup.tsx	✅
superset-frontend/src/dashboard/styles.ts	✅
superset-frontend/src/GlobalStyles.tsx	✅
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterPopoverTrigger/index.tsx	✅
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx	✅
superset-frontend/src/explore/components/controls/MapViewControl/MapViewControl.tsx	✅
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopoverTrigger.tsx	✅
superset-frontend/src/explore/components/controls/MapViewControl/MapViewPopoverContent.tsx	✅
superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx	✅
superset-frontend/src/theme/index.ts	✅
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx	✅
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx	✅
superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.tsx	✅
superset-frontend/src/explore/components/DataTableControl/index.tsx	✅
superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.jsx	✅
superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx	✅
superset-frontend/cypress-base/cypress/support/e2e.ts	✅
superset-frontend/src/components/DropdownContainer/index.tsx	✅
superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts	✅
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx	✅
superset-frontend/cypress-base/cypress/support/directories.ts	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

Setting Value

Review Schedule Automatic excluding drafts

Max Issue Count 10

Automatic PR Descriptions ❌

Issue Categories

Category Enabled

Naming ✅

Database Operations ✅

Documentation ✅

Logging ✅

Error Handling ✅

Systems and Environment ✅

Objects and Data Structures ✅

Readability and Maintainability ✅

Asynchronous Processing ✅

Design Patterns ✅

Third-Party Libraries ✅

Performance ✅

Security ✅

Functionality ✅

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ [email protected]

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

korbit-ai · 2025-01-24T13:11:35Z

superset-frontend/cypress-base/cypress/support/e2e.ts

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


Unreliable Header Access

Tell me more

What is the issue?

The access of 'location' property directly from response.headers might be unreliable as some browsers normalize header names to lowercase while others preserve case sensitivity.

Why this matters

Different browsers handle HTTP header case sensitivity differently. The direct property access may lead to inconsistent behavior across different browsers and environments, potentially breaking the login functionality.

Suggested change ∙ Feature Preview

Use a case-insensitive method to access the header value:

const location = response.headers.get('location') || response.headers.get('Location');

_{💡 Does this comment miss the mark? Tell us why and Korbit will adapt to your team’s feedback.

💬 Chat with Korbit by mentioning @korbit-ai.}

korbit-ai · 2025-01-24T13:11:36Z

superset-frontend/src/theme/index.ts

@@ -181,6 +181,7 @@ const baseConfig: ThemeConfig = {
    },
    Popover: {
      colorBgElevated: supersetTheme.colors.grayscale.light5,
+      zIndexPopup: supersetTheme.zIndex.max,


Redundant Z-Index Configuration

Tell me more

What is the issue?

The zIndexPopup value is set redundantly in multiple component configurations (Dropdown, Menu, Popover) when it's already defined at the root level with zIndexPopupBase.

Why this matters

Having multiple identical z-index definitions can lead to inconsistencies if the base value needs to be changed in the future. The component-level zIndexPopup values are unnecessary since they inherit from zIndexPopupBase.

Suggested change ∙ Feature Preview

Remove the redundant zIndexPopup definitions from individual components since they inherit from the base configuration:

const baseConfig: ThemeConfig = { token: { zIndexPopupBase: supersetTheme.zIndex.max, }, components: { Dropdown: { colorBgElevated: supersetTheme.colors.grayscale.light5, // remove zIndexPopup }, Menu: { // ... other properties ... // remove zIndexPopup }, Popover: { colorBgElevated: supersetTheme.colors.grayscale.light5, // remove zIndexPopup }, } }

_{💡 Does this comment miss the mark? Tell us why and Korbit will adapt to your team’s feedback.

💬 Chat with Korbit by mentioning @korbit-ai.}

korbit-ai · 2025-01-24T13:11:36Z

superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx

+        {/* <Global
          styles={css`
            // @z-index-above-dashboard-header (100) + 1 = 101
            ${fullSizeChartId &&
            `div > .filterStatusPopover.ant-popover{z-index: 101}`}
          `}
-        />
+        /> */}


Filter popover z-index issue in full-size mode

Tell me more

What is the issue?

The commented-out Global styles for the filterStatusPopover z-index may affect the visibility of filter popover when a chart is in full-size mode.

Why this matters

Without the z-index styling, the filter status popover might appear behind the full-size chart, making it inaccessible to users when they need to interact with filters while viewing a chart in full-size mode.

Suggested change ∙ Feature Preview

Re-enable the Global styles component to maintain proper z-index layering:

<Global styles={css` // @z-index-above-dashboard-header (100) + 1 = 101 ${fullSizeChartId && `div > .filterStatusPopover.ant-popover{z-index: 101}`} `} />

_{💡 Does this comment miss the mark? Tell us why and Korbit will adapt to your team’s feedback.

💬 Chat with Korbit by mentioning @korbit-ai.}

alexandrusoare added 7 commits January 21, 2025 10:14

refactor(Popover): upgrade Popover to antd5

be1d564

refactor(DetailsPanel): removing custom styles

f88fa0e

refactor(components): upgrading to andt5

e6cc1fe

refactor(popover): updating to Antd5

636c1a1

refactor(popover): updating to Antd5

3c99830

refactor(Popover): upgrading to antd5

a69a8b0

refactor(Popover): upgrading to antd5

584239b

pull-request-size bot added the size/L label Jan 23, 2025

github-actions bot added the packages label Jan 23, 2025

rusackas added the review:draft label Jan 23, 2025

alexandrusoare and others added 7 commits January 24, 2025 10:23

Merge branch 'master' into alexandrusoare/refactor/antd5-popover

2839455

fix(e2e): fixed e2e test

74d9a30

fix(e2e): fixed e2e test

40a2034

feat(story): added props to storybook

cabf53f

feat(styles): removed some styles

159a7fb

feat(style): removed some styles

4677efa

feat(style): removed some styles

18ef69e

alexandrusoare marked this pull request as ready for review January 24, 2025 13:04

alexandrusoare requested review from sadpandajoe, geido, eschutho, rusackas, betodealmeida, michael-s-molina and kgabryje as code owners January 24, 2025 13:04

dosubot bot added change:frontend Requires changing the frontend frontend:refactor:antd5 labels Jan 24, 2025

korbit-ai bot reviewed Jan 24, 2025

View reviewed changes

michael-s-molina removed the review:draft label Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(Popover): Upgrade Popover to Antd5 #31973

refactor(Popover): Upgrade Popover to Antd5 #31973

alexandrusoare commented Jan 23, 2025 •

edited by michael-s-molina

Loading

korbit-ai bot commented Jan 23, 2025

korbit-ai bot left a comment •

edited

Loading

korbit-ai bot Jan 24, 2025

korbit-ai bot Jan 24, 2025

korbit-ai bot Jan 24, 2025

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Category	Enabled
Naming	✅
Database Operations	✅
Documentation	✅
Logging	✅
Error Handling	✅
Systems and Environment	✅
Objects and Data Structures	✅
Readability and Maintainability	✅
Asynchronous Processing	✅
Design Patterns	✅
Third-Party Libraries	✅
Performance	✅
Security	✅
Functionality	✅

refactor(Popover): Upgrade Popover to Antd5 #31973

Are you sure you want to change the base?

refactor(Popover): Upgrade Popover to Antd5 #31973

Conversation

alexandrusoare commented Jan 23, 2025 • edited by michael-s-molina Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

BEFORE

AFTER

BEFORE

AFTER

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

korbit-ai bot commented Jan 23, 2025

korbit-ai bot left a comment • edited Loading

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Interacting with Korbit

Customizing Korbit

Current Korbit Configuration

Feedback and Support

Looking to add Korbit to your team? Get started with a free 2 week trial here

korbit-ai bot Jan 24, 2025

Choose a reason for hiding this comment

Unreliable Header Access

What is the issue?

Why this matters

Suggested change ∙ Feature Preview

korbit-ai bot Jan 24, 2025

Choose a reason for hiding this comment

Redundant Z-Index Configuration

What is the issue?

Why this matters

Suggested change ∙ Feature Preview

korbit-ai bot Jan 24, 2025

Choose a reason for hiding this comment

Filter popover z-index issue in full-size mode

What is the issue?

Why this matters

Suggested change ∙ Feature Preview

alexandrusoare commented Jan 23, 2025 •

edited by michael-s-molina

Loading

korbit-ai bot left a comment •

edited

Loading