-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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(Menu): Upgrade Menu Component to Ant Design 5 #31699
base: master
Are you sure you want to change the base?
Conversation
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
|
…o/refactor/antd5-upgrade-menu
2e268ad
to
c595454
Compare
03f5cfd
to
690dfeb
Compare
690dfeb
to
effeffc
Compare
…o/refactor/antd5-upgrade-menu
/korbit-review |
There was a problem hiding this 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 |
---|---|---|
Submenu collapse on hover ▹ view | ||
Incorrect Menu Overflow Prop Name ▹ view | ||
Premature Menu Closure ▹ view | ||
Incorrect Dropdown State Management ▹ view | ||
Missing Menu Item Keys ▹ view | ||
Outdated Submenu Arrow Selectors ▹ view | ||
Missing Active Menu Item Indicator ▹ view | ||
Silent Failure with Empty Object Fallback ▹ view |
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/src/components/Chart/MenuItemWithTruncation.tsx | ✅ |
superset-frontend/src/features/home/LanguagePicker.stories.tsx | ✅ |
superset-frontend/src/components/Menu/Menu.stories.tsx | ✅ |
superset-frontend/src/GlobalStyles.tsx | ✅ |
superset-frontend/src/components/PopoverDropdown/index.tsx | ✅ |
superset-frontend/src/views/menu.tsx | ✅ |
superset-frontend/src/components/ModalTrigger/index.tsx | ✅ |
superset-frontend/src/dashboard/styles.ts | ✅ |
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx | ✅ |
superset-frontend/src/components/Table/cell-renderers/ActionCell/index.tsx | ✅ |
superset-frontend/src/components/Dropdown/index.tsx | ✅ |
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx | ✅ |
superset-frontend/src/components/DropdownSelectableIcon/index.tsx | ✅ |
superset-frontend/src/theme/index.ts | ✅ |
superset-frontend/src/features/reports/ReportModal/HeaderReportDropdown/index.tsx | ✅ |
superset-frontend/src/components/Chart/DrillDetail/DrillDetailMenuItems.tsx | ✅ |
superset-frontend/src/components/Menu/index.tsx | ✅ |
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx | ✅ |
superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/index.tsx | ✅ |
superset-frontend/src/pages/Home/index.tsx | ✅ |
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx | ✅ |
superset-frontend/src/features/home/SubMenu.tsx | ✅ |
superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx | ✅ |
superset-frontend/src/features/home/Menu.tsx | ✅ |
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx | ✅ |
superset-frontend/src/features/home/RightMenu.tsx | ✅ |
superset-frontend/cypress-base/cypress/support/directories.ts | ✅ |
superset-frontend/src/pages/DatasetList/index.tsx | ✅ |
superset-frontend/src/dashboard/components/SliceHeaderControls/index.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
<Menu.SubMenu | ||
title={title} | ||
key={key} | ||
onTitleMouseEnter={() => setOpenKeys?.(undefined)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submenu collapse on hover
Tell me more
What is the issue?
Setting openKeys to undefined on mouse enter may cause unexpected menu behavior, disrupting the user's ability to navigate through submenus.
Why this matters
When hovering over the submenu title, it will reset the open state, which could make the menu collapse unexpectedly and create a poor user experience.
Suggested change ∙ Feature Preview
Remove the onTitleMouseEnter handler as it's not a standard pattern in Ant Design menus and could cause navigation issues.
<Menu.SubMenu
title={title}
key={key}
>
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
@@ -255,7 +194,7 @@ const SubMenuComponent: FunctionComponent<SubMenuProps> = props => { | |||
<StyledHeader> | |||
<Row className="menu" role="navigation"> | |||
{props.name && <div className="header">{props.name}</div>} | |||
<Menu mode={showMenu} style={{ backgroundColor: 'transparent' }}> | |||
<Menu mode={showMenu} disabledOverflow> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect Menu Overflow Prop Name
Tell me more
What is the issue?
The 'disabledOverflow' prop is being used on Menu components but it should be 'overflowedIndicator={null}' in Ant Design v5.
Why this matters
Using an incorrect prop name will cause the overflow behavior to not work as intended, potentially causing menu items to be incorrectly displayed when there's insufficient space.
Suggested change ∙ Feature Preview
Replace 'disabledOverflow' with 'overflowedIndicator={null}' in the Menu components:
<Menu mode={showMenu} overflowedIndicator={null}>
{/* ... */}
</Menu>
<Menu mode="horizontal" triggerSubMenuAction="click" overflowedIndicator={null}>
{/* ... */}
</Menu>
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
onClick={() => { | ||
setVisible(false); | ||
onClose(); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Premature Menu Closure
Tell me more
What is the issue?
The onClick handler in Menu component closes the dropdown menu for all menu item clicks, including when opening submenus, which disrupts the user interaction flow.
Why this matters
Users will be unable to navigate through nested menu items as the menu closes immediately upon any click, preventing access to submenu options.
Suggested change ∙ Feature Preview
Move the click handler to individual menu items that should trigger closure, or check if the clicked item has a submenu before closing:
onClick={({ key }) => {
// Only close if not a submenu item
if (!openKeys.includes(key)) {
setVisible(false);
onClose();
}
}}
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
Show resolved
Hide resolved
superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts
Outdated
Show resolved
Hide resolved
superset-frontend/cypress-base/cypress/e2e/dashboard/drilltodetail.test.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/DropdownSelectableIcon/index.tsx
Outdated
Show resolved
Hide resolved
…est.ts Co-authored-by: Beto Dealmeida <[email protected]>
…tail.test.ts Co-authored-by: Beto Dealmeida <[email protected]>
Co-authored-by: Beto Dealmeida <[email protected]>
SUMMARY
Migration of Menu component from Ant Design v4 -> v5.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION