Skip to content

Conversation

@EnxDev
Copy link
Contributor

@EnxDev EnxDev commented Jun 5, 2025

SUMMARY

This pull request makes minor UI tweaks to address regressions caused by the theming project.
The goal is to restore visual consistency and ensure components match the intended design.
No functional logic has been impacted.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  • All tests should pass

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
Copy link

korbit-ai bot commented Jun 5, 2025

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

@EnxDev EnxDev added change:frontend Requires changing the frontend frontend:refactor Related to refactoring the frontend frontend:refactor:antd5 review:draft labels Jun 5, 2025
@EnxDev EnxDev requested review from kgabryje and msyavuz June 10, 2025 09:33
@EnxDev EnxDev marked this pull request as ready for review June 10, 2025 09:33
@github-actions
Copy link
Contributor

@EnxDev Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

I've completed my review and didn't find any issues.

Files scanned
File Path Reviewed
superset-frontend/src/dashboard/components/DeleteComponentButton.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
superset-frontend/src/components/Datasource/Field.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx
superset-frontend/src/components/MessageToasts/Toast.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx
superset-frontend/src/dashboard/components/gridComponents/Column.jsx
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx
superset-frontend/src/components/Datasource/CollectionTable.tsx
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
superset-frontend/cypress-base/cypress/support/directories.ts
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
superset-frontend/src/components/Datasource/DatasourceEditor.jsx

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

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

Loving Korbit!? Share us on LinkedIn Reddit and X

@EnxDev EnxDev requested a review from kasiazjc June 10, 2025 09:51
paddingTop: theme.sizeUnit * 3,
position: 'sticky',
bottom: theme.sizeUnit,
marginBottom: theme.sizeUnit * 3,
Copy link
Member

Choose a reason for hiding this comment

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

Is this intended?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is. It causes buttons to not be sticky.
Before:
image

After:
image

Copy link
Contributor Author

@EnxDev EnxDev Jun 12, 2025

Choose a reason for hiding this comment

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

I thought you were referring to the * 3, Good catch!

@EnxDev EnxDev requested a review from msyavuz June 10, 2025 20:27
@github-actions
Copy link
Contributor

@msyavuz Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

@github-actions
Copy link
Contributor

@msyavuz Ephemeral environment spinning up at http://54.202.174.181:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

@github-actions
Copy link
Contributor

@EnxDev Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

@github-actions
Copy link
Contributor

@EnxDev Ephemeral environment spinning up at http://35.167.61.154:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

paddingTop: theme.sizeUnit * 3,
position: 'sticky',
bottom: theme.sizeUnit,
marginBottom: theme.sizeUnit * 3,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is. It causes buttons to not be sticky.
Before:
image

After:
image

await userEvent.type(inputCertifiedBy, 'test');
await userEvent.type(inputCertDetails, 'test');
});
}, 60000); // 60 seconds timeout to avoid timeouts
Copy link
Member

Choose a reason for hiding this comment

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

60 seconds is a bit much, if we decide to increase timeout let's do it in smaller increments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a 60-second timeout, exactly like it was done in the test right after the one called "can delete columns".
I believe this is just a temporary fix to avoid blocking the tests. The entire test likely needs to be reviewed; for example, some await statements that have no effect should probably be removed.

@EnxDev EnxDev requested a review from msyavuz June 12, 2025 14:27
Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

MERGE AWAY!

@EnxDev EnxDev merged commit 253a72f into template_less Jun 12, 2025
49 checks passed
@EnxDev EnxDev deleted the enxdev/fix/theming/visual-regressions-p4 branch June 12, 2025 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend frontend:refactor:antd5 frontend:refactor Related to refactoring the frontend global:theming Related to theming Superset packages size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants