refactor(Collapse): Upgrade Collapse to Antd5 #32959

DamianPendrak · 2025-04-01T12:28:38Z

SUMMARY

Fixes #32542

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Home

Before

After

SQL Editor Table Element

Before

After

Explore Data

Before

After

Connect a database - Advanced tab

Before

After

Upload modal

Before

After

Add Alert / Report

Before

After

Fixed or metric

Before

After

Dashboard filters

Before

After

Dashboard Cross-filters

Before

After

Dasbhoard - Out of scope filters

Before

After

VizTypeGallery

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: Ant Design 5: Upgrade Collapse component #32542
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-04-01T12:28:43Z

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

geido · 2025-04-02T11:16:37Z

superset-frontend/src/SqlLab/components/TableElement/index.tsx

-                aria-label="Copy"
-                tooltip={t('Copy SELECT statement to the clipboard')}
+                className={
+                  `fa fa-sort-${sortColumns ? 'numeric' : 'alpha'}-asc ` +


We are removing usages of fa icons from the codebase. Can you use the Icons component from src/components/Icons instead?

geido · 2025-04-02T11:17:44Z

superset-frontend/src/SqlLab/components/TableElement/index.tsx

+                <CopyToClipboard
+                  copyNode={
+                    <IconTooltip
+                      aria-label="Copy"


Let's localize this too. I see inconsistent localization with this prop across the codebase but I guess localizing won't hurt.

geido · 2025-04-02T11:19:10Z

superset-frontend/src/components/Collapse/index.tsx

-          expandIconPosition === 'right' &&
-          `
-            .anticon.anticon-right.ant-collapse-arrow > svg {
+      ${({ expandIconPosition }) =>


This is all necessary because Ant Design does not support arrows on the right?

It does, but the arrow points to the right by default. In Superset, when the arrow is on the right, it points down when closed and up when open

geido · 2025-04-02T11:20:30Z

superset-frontend/src/explore/components/controls/FixedOrMetricControl/index.jsx

-        </Collapse>
+          items={[
+            {
+              key: '1',


Can we use human-readable keys?

geido · 2025-04-02T11:22:09Z

...t-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx

+    <Collapse
+      items={[
+        {
+          key: '1',


human-readable if possible

geido · 2025-04-02T11:22:34Z

superset-frontend/src/pages/Home/index.tsx

-                  {!queryData ? (
+              items={[
+                {
+                  key: '1',


same comment about human-readable

msyavuz · 2025-04-02T20:04:08Z

superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx

+          {tables.map(table => (
+            <TableElement
+              table={table}
+              key={table.id}
+              activeKey={tables
+                .filter(({ expanded }) => expanded)
+                .map(({ id }) => id)}
+              onChange={onToggleTable}
+            />
+          ))}


I am not sure why this refactor of Collapse into TableElement necessary?

Mainly because of the hover functionality for the tooltip and the fact that the Collapse items prop accepts label and children as separate components

msyavuz · 2025-04-02T20:06:57Z

superset-frontend/src/SqlLab/components/TableElement/index.tsx

+              >
+                <Icons.SyncOutlined
+                  iconSize="m"
+                  iconColor={theme.colors.grayscale.dark5}


I think we are trying to use Ant Design tokens whenever we can so i am not sure but this would roughly map to theme.colorIcon?

Okay, I thought that the contrast is too low with the theme.colorIcon. I will update the code, but let me know if something changes because of the contrast

msyavuz · 2025-04-02T20:07:23Z

superset-frontend/src/SqlLab/components/TableElement/index.tsx

                  aria-hidden
+                  iconColor={
+                    sortColumns
+                      ? theme.colors.grayscale.dark5


msyavuz · 2025-04-02T20:07:38Z

superset-frontend/src/SqlLab/components/TableElement/index.tsx

+                      <Icons.CopyOutlined
+                        iconSize="m"
+                        aria-hidden
+                        iconColor={theme.colors.grayscale.dark5}


msyavuz · 2025-04-02T20:07:45Z

superset-frontend/src/SqlLab/components/TableElement/index.tsx

+                <Icons.CloseOutlined
+                  iconSize="m"
+                  aria-hidden
+                  iconColor={theme.colors.grayscale.dark5}


msyavuz · 2025-04-02T20:09:39Z

superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.tsx

@@ -41,9 +41,11 @@ const collapseStyle = (theme: SupersetTheme) => css`
  .ant-collapse-arrow {
    left: 0px !important;
  }
+
  .ant-collapse-header {


Won't these classnames change to antd5?

msyavuz · 2025-04-02T20:12:55Z

...ashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

+                  {
+                    key: `${filterId}-${FilterPanels.configuration.key}`,
+                    label: FilterPanels.configuration.name,
+                    children: (


Is losing forceRender not important here?

Restored the forceRender

geido · 2025-04-03T12:46:31Z

superset-frontend/src/SqlLab/components/ColumnElement/index.tsx

@@ -105,7 +105,7 @@ const ColumnElement = ({ column }: ColumnElementProps) => {
  }
  return (
    <div className="clearfix table-column">
-      <div className="pull-left m-l-10 col-name">
+      <div className="pull-left col-name">


Do we know where these classes are coming from? Can we use the standard Ant Design approach here, like Flex or Space?

geido · 2025-04-03T12:49:57Z

superset-frontend/src/SqlLab/components/TableElement/index.tsx

-      }
-    }
-  `}
+const StyledControls = styled.div`


Don't want to go too much outside of the scope of the PR but do you think we can use standard Ant Design Flex and Space components to manage the layout for all the styles in this file?

geido · 2025-04-03T12:51:52Z

...-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/VerticalCollapse.tsx

+                  width: 100%;
+                  height: 1px;
+                  display: block;
+                  background: ${theme.colors.grayscale.light3};


Do we need this? Can we use an Ant Design color instead? Also, this should be a standard atomic component?

mistercrunch · 2025-04-03T18:21:55Z

@DamianPendrak I see some pre-commit checks fail, highly recommend setting up pre-commit -> https://superset.apache.org/docs/contributing/development/#git-hooks

Once you do, you may want to squish your commits as a way to re-trigger it on all the files you touched in this PR.

mistercrunch · 2025-04-03T18:24:04Z

Also wondering what's required to do in order for going from a DRAFT PR to "ready for review"

DamianPendrak · 2025-04-03T20:33:50Z

Also wondering what's required to do in order for going from a DRAFT PR to "ready for review"

I think I will fix the failing tests and then mark it ready

korbit-ai · 2025-04-04T08:50:43Z

Korbit doesn't automatically review large (3000+ lines changed) pull requests such as this one. If you want me to review anyway, use /korbit-review.

geido · 2025-04-07T14:16:00Z

The color of the subtitle seems hard to see. Is this a vanilla Ant Design implementation?
When I first visited the homepage I found all collapse closed. I don't believe I closed them. Something to check.
Colors of text inside the collapse for filters is off. Is it because of Checkbox? Currently being developed here

You have a conflict. Thanks for the hard work on this!

geido

Approving as it looks mostly good with comments I have left above. It would be great to have more eyes on testing as well. @msyavuz maybe you can help with that.

Thank you!

mistercrunch · 2025-04-07T19:42:19Z

NICE! Looking forward to merge this one - the theming branch (template_less) is really coming together!

DamianPendrak · 2025-04-07T19:56:14Z

The color of the subtitle seems hard to see. Is this a vanilla Ant Design implementation?

When I first visited the homepage I found all collapse closed. I don't believe I closed them. Something to check.

Colors of text inside the collapse for filters is off. Is it because of Checkbox? Currently being developed here

You have a conflict. Thanks for the hard work on this!

Fixed except for the Checkbox text color. This is the default Checkbox from Ant Design without custom styles.

mistercrunch · 2025-04-07T20:08:31Z

I just fixed the merge conflict in hope to merge this today. NOTE: alphabetizing Icons names would help reduce the odds of conflicts. Looks like it's mostly alphabetized now, but putting the latest icon at the bottom of the list will lead to more merge-conflicts than squeezing it in the list where it belongs.

mistercrunch · 2025-04-07T20:33:28Z

Looks like there's a single unit test failing here ^^^

DamianPendrak · 2025-04-07T21:03:44Z

Looks like there's a single unit test failing here ^^^

Can we re-trigger the tests? They are passing locally and I believe the failing test is not related to the Collapse component. This happened before with this unit test and re-run fixed it

mistercrunch · 2025-04-07T21:24:42Z

triggered 🤞 🤞

mistercrunch · 2025-04-07T21:37:30Z

Some styling issues post-merge ->

mistercrunch · 2025-04-07T21:39:14Z

Not sure if we want to preserve previous styling in modals (full width, no rounded corners, ...) or go vanilla. Thought the custom styles in modal was slick, but if we decide to maintain it the styling should be implemented as mode prop into the base component.

DamianPendrak · 2025-04-08T13:57:59Z

DatabaseModal fix: #33038

pull-request-size bot added the size/XXL label Apr 1, 2025

sadpandajoe added review:checkpoint Last PR reviewed during the daily review standup review:draft labels Apr 1, 2025

sadpandajoe linked an issue Apr 1, 2025 that may be closed by this pull request

Ant Design 5: Upgrade Collapse component #32542

Closed

geido added preset:bounty Issues that have been selected by Preset and have a bounty attached. hold! On hold labels Apr 2, 2025

geido reviewed Apr 2, 2025

View reviewed changes

msyavuz self-requested a review April 2, 2025 11:29

DamianPendrak force-pushed the collapse-component-upgrade branch from 2eb2f43 to a331275 Compare April 2, 2025 13:00

sadpandajoe removed the review:checkpoint Last PR reviewed during the daily review standup label Apr 2, 2025

trentlavoie mentioned this pull request Apr 2, 2025

fix(frontend): add missing antd-5 icon to import #32990

Merged

9 tasks

msyavuz reviewed Apr 2, 2025

View reviewed changes

geido reviewed Apr 3, 2025

View reviewed changes

DamianPendrak marked this pull request as ready for review April 4, 2025 08:50

DamianPendrak requested review from sadpandajoe, eschutho, rusackas, betodealmeida and mistercrunch as code owners April 4, 2025 08:50

dosubot bot added change:frontend Requires changing the frontend frontend:refactor:antd5 labels Apr 4, 2025

DamianPendrak force-pushed the collapse-component-upgrade branch from 0d03d4d to bb7a638 Compare April 4, 2025 10:09

geido approved these changes Apr 7, 2025

View reviewed changes

Collapse component upgrade to antd5

b6013f8

DamianPendrak force-pushed the collapse-component-upgrade branch from bb7a638 to b6013f8 Compare April 7, 2025 19:31

Merge branch 'template_less' into collapse-component-upgrade

fa79d09

mistercrunch removed the hold! On hold label Apr 7, 2025

mistercrunch merged commit 3950cf0 into apache:template_less Apr 7, 2025
41 of 42 checks passed

DamianPendrak mentioned this pull request Apr 8, 2025

fix: upgrade tabs in database modal #33038

Open

9 tasks

refactor(Collapse): Upgrade Collapse to Antd5 #32959

refactor(Collapse): Upgrade Collapse to Antd5 #32959

Conversation

DamianPendrak commented Apr 1, 2025 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Home

Before

After

SQL Editor Table Element

Before

After

Explore Data

Before

After

Connect a database - Advanced tab

Before

After

Upload modal

Before

After

Add Alert / Report

Before

After

Fixed or metric

Before

After

Dashboard filters

Before

After

Dashboard Cross-filters

Before

After

Dasbhoard - Out of scope filters

Before

After

VizTypeGallery

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

korbit-ai bot commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Apr 3, 2025 • edited Loading

mistercrunch commented Apr 3, 2025

DamianPendrak commented Apr 3, 2025

korbit-ai bot commented Apr 4, 2025

geido commented Apr 7, 2025

geido left a comment

Choose a reason for hiding this comment

mistercrunch commented Apr 7, 2025

DamianPendrak commented Apr 7, 2025

mistercrunch commented Apr 7, 2025

mistercrunch commented Apr 7, 2025

DamianPendrak commented Apr 7, 2025

mistercrunch commented Apr 7, 2025

mistercrunch commented Apr 7, 2025

mistercrunch commented Apr 7, 2025

DamianPendrak commented Apr 8, 2025

DamianPendrak commented Apr 1, 2025 •

edited

Loading

mistercrunch commented Apr 3, 2025 •

edited

Loading