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(Radio): Upgrade Radio Component to Ant Design 5 #32004

Merged
merged 18 commits into from
Jan 31, 2025

Conversation

EnxDev
Copy link
Contributor

@EnxDev EnxDev commented Jan 27, 2025

SUMMARY

Migration of Radio component from Ant Design v4 to Ant Design v5.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

  • Calendar frame before
    Screenshot 2025-01-25 181611

  • CalendarFrame after
    Screenshot 2025-01-27 213626

  • CommonFrame before
    Screenshot 2025-01-25 183010

  • CommonFrame after
    Screenshot 2025-01-25 183220

  • CurrentFrame before
    Screenshot 2025-01-25 183925

  • CurrentFrame after
    Screenshot 2025-01-25 183737

  • DisplayModeToggle before
    Screenshot 2025-01-25 185257

  • DisplayModeToggle after
    Screenshot 2025-01-25 193315

Recording.2025-01-25.193403.mp4
  • HeaderWithRadioGroup before
    Screenshot 2025-01-25 211904

  • HeaderWithRadioGroup after
    Screenshot 2025-01-25 212956

  • ReportModal before
    Screenshot 2025-01-27 173413

  • ReportModal after
    Screenshot 2025-01-27 214013

  • SaveModal before
    Screenshot 2025-01-25 213500

  • SaveModal After
    Screenshot 2025-01-25 213500

  • Before
    Screenshot 2025-01-25 211904

before.mp4
  • After
    Screenshot 2025-01-25 212956
after.mp4

TESTING INSTRUCTIONS

  1. CI should pass
  2. All existing Radios should work as expected with minimal visual differences

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

- Implements Radio from Antd5
- Update types in constants file for options constants
- Radio.GroupWrapper customization for Radio.Group component
- Replaces Radio.Group in the base code with Radio.GroupWrapper
- Optimizes by removing excess code
@dosubot dosubot bot added change:frontend Requires changing the frontend frontend:refactor:antd5 labels Jan 27, 2025
@EnxDev EnxDev marked this pull request as draft January 27, 2025 20:49
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.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Fix Detected
Functionality Undefined Radio Component Reference ▹ view

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.

Feedback and Support

- Improves reusability for the Radio Component
- Aligns storybook with component changes
@EnxDev EnxDev marked this pull request as ready for review January 29, 2025 11:26
@EnxDev EnxDev requested a review from geido January 29, 2025 11:26
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.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Fix Detected
Functionality Missing enableSpaceConfig Control ▹ view
Files scanned
File Path Reviewed
superset-frontend/src/explore/components/controls/DateFilterControl/components/CalendarFrame.tsx
superset-frontend/src/explore/components/controls/DateFilterControl/components/CommonFrame.tsx
superset-frontend/src/explore/components/controls/DateFilterControl/components/CurrentCalendarFrame.tsx
superset-frontend/src/components/Chart/DrillBy/useDisplayModeToggle.tsx
superset-frontend/src/components/Table/header-renderers/HeaderWithRadioGroup.tsx
superset-frontend/src/components/Radio/index.tsx
superset-frontend/src/explore/components/controls/DateFilterControl/utils/constants.ts
superset-frontend/src/components/Radio/Radio.stories.tsx
superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx
superset-frontend/src/explore/components/DataTableControl/index.tsx
superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx
superset-frontend/cypress-base/cypress/support/e2e.ts
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

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

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

superset-frontend/src/components/Radio/Radio.stories.tsx Outdated Show resolved Hide resolved
@kgabryje
Copy link
Member

Looks like the radio buttons in drill by are separated

image

@EnxDev
Copy link
Contributor Author

EnxDev commented Jan 30, 2025

Looks like the radio buttons in drill by are separated

image

Yes, when I modified the Radio.GroupWrapper, it broke the spacing of the drill-by. I reattached it again, thanks

- small changes from PR reviews
- removes children from Radio.GroupWrapper
Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

lgtm

@EnxDev EnxDev merged commit 468bb5f into master Jan 31, 2025
47 of 48 checks passed
@EnxDev EnxDev deleted the refactor/antd5-radio branch January 31, 2025 16:45
@michael-s-molina
Copy link
Member

I believe this broke master CI.

Screenshot 2025-01-31 at 15 54 45

We need to try to fix it before Monday when we'll cut 5.0. Otherwise, we'll need to revert.

@michael-s-molina
Copy link
Member

michael-s-molina commented Jan 31, 2025

It looks like this was a false alarm and we have another flaky test 😞. It succeeded on the third run. No changes required and thanks for the contribution @EnxDev!

tmsjordan pushed a commit to tmsdevelopment/superset that referenced this pull request Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants