korbit-ai[bot] commented on code in PR #31699:
URL: https://github.com/apache/superset/pull/31699#discussion_r1915087620
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -495,22 +645,15 @@ const SliceHeaderControls = (props:
SliceHeaderControlsProps) => {
/>
)}
<NoAnimationDropdown
- overlay={menu}
+ dropdownRender={() => menu}
overlayStyle={dropdownOverlayStyle}
trigger={['click']}
placement="bottomRight"
- visible={dropdownIsOpen}
- onVisibleChange={status => toggleDropdown({ close: !status })}
- onKeyDown={e =>
- handleDropdownNavigation(
- e,
- dropdownIsOpen,
- menu,
- toggleDropdown,
- setSelectedKeys,
- setOpenKeys,
- )
- }
+ open={dropdownIsOpen}
+ autoFocus
+ onOpenChange={status => toggleDropdown({ close: !status })}
Review Comment:
### Incorrect Dropdown State Management <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The dropdown's onOpenChange handler inverts the status, which could lead to
incorrect dropdown state when the dropdown is closed by clicking outside.
###### Why this matters
The inverted logic can cause the dropdown to get out of sync with its actual
visible state when external closing events occur.
###### Suggested change ∙ *Feature Preview*
Directly use the status value:
```typescript
onOpenChange={status => setDropdownIsOpen(status)}
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:eabad3f2-7b51-4e47-b985-df2a352d6aa8 -->
##########
superset-frontend/src/theme/index.ts:
##########
@@ -145,6 +150,19 @@ const baseConfig: ThemeConfig = {
colorSplit: supersetTheme.colors.grayscale.light3,
colorText: supersetTheme.colors.grayscale.dark1,
},
+ Menu: {
+ itemHeight: 32,
+ colorBgContainer: supersetTheme.colors.grayscale.light5,
+ subMenuItemBg: supersetTheme.colors.grayscale.light5,
+ colorBgElevated: supersetTheme.colors.grayscale.light5,
+ boxShadowSecondary: `0 3px 6px -4px
${addAlpha(supersetTheme.colors.grayscale.dark2, 0.12)}, 0 6px 16px 0
${addAlpha(supersetTheme.colors.grayscale.dark2, 0.08)}, 0 9px 28px 8px
${addAlpha(supersetTheme.colors.grayscale.dark2, 0.05)}`,
+ activeBarHeight: 0,
Review Comment:
### Missing Active Menu Item Indicator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Setting activeBarHeight to 0 completely removes the active indicator bar in
the menu, which reduces visual feedback for the selected item.
###### Why this matters
Without a visual indicator for the active menu item, users may have
difficulty identifying which menu item is currently selected, impacting
navigation usability.
###### Suggested change ∙ *Feature Preview*
Restore the active indicator bar with a minimal height:
```typescript
activeBarHeight: 2,
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:2bf396c3-60f8-4b07-931b-fa4526f6a7d9 -->
##########
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx:
##########
@@ -95,28 +97,18 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {
}
return (
- <Menu
- selectable={false}
- selectedKeys={selectedKeys}
- onClick={e =>
- e.key === MenuKeys.CopyLink ? onCopyLink() : onShareByEmail()
- }
+ <Menu.SubMenu
+ title={title}
+ key={key}
+ onTitleMouseEnter={() => setOpenKeys?.(undefined)}
Review Comment:
### Submenu collapse on hover <sub></sub>
<details>
<summary>Tell me more</summary>
###### 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.
```typescript
<Menu.SubMenu
title={title}
key={key}
>
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:4a1956bc-d0f6-4505-9914-bf09eec1cfec -->
##########
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx:
##########
@@ -279,37 +286,58 @@ const ChartContextMenu = (
);
return ReactDOM.createPortal(
- <Dropdown
- overlay={
- <Menu
- className="chart-context-menu"
- data-test="chart-context-menu"
- onOpenChange={openKeys => {
- setOpenKeys(openKeys);
- }}
- >
- {menuItems.length ? (
- menuItems
- ) : (
- <Menu.Item disabled>No actions</Menu.Item>
- )}
- </Menu>
- }
- trigger={['click']}
- onVisibleChange={value => !value && onClose()}
- >
- <span
- id={`hidden-span-${id}`}
- css={{
- visibility: 'hidden',
- position: 'fixed',
- top: clientY,
- left: clientX,
- width: 1,
- height: 1,
+ <>
+ <Dropdown
+ overlay={
+ <Menu
+ className="chart-context-menu"
+ data-test="chart-context-menu"
+ onOpenChange={setOpenKeys}
+ onClick={() => {
+ setVisible(false);
+ onClose();
+ }}
Review Comment:
### Premature Menu Closure <sub></sub>
<details>
<summary>Tell me more</summary>
###### 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:
```typescript
onClick={({ key }) => {
// Only close if not a submenu item
if (!openKeys.includes(key)) {
setVisible(false);
onClose();
}
}}
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:8ea1c828-09e8-4665-8985-61589e29f1d6 -->
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -98,7 +98,7 @@ export default function DrillDetailModal({
const dashboardPageId = useContext(DashboardPageIdContext);
const { slice_name: chartName } = useSelector(
(state: { sliceEntities: { slices: Record<number, Slice> } }) =>
- state.sliceEntities.slices[chartId],
+ state.sliceEntities?.slices?.[chartId] || {},
Review Comment:
### Silent Failure with Empty Object Fallback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error case where sliceEntities or slices is undefined is silently
handled with an empty object fallback, losing potential error context.
###### Why this matters
This silent failure makes debugging harder when there are actual issues with
the state structure or chartId, as it masks the real problem by returning an
empty object.
###### Suggested change ∙ *Feature Preview*
Consider logging a warning or throwing an error when expected state
structure is missing:
```typescript
const slices = state.sliceEntities?.slices;
if (!slices) {
console.warn(`Missing slices data for chart ${chartId}`);
}
return slices?.[chartId] || {};```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:ca1db5a1-fc7e-4aea-aef4-651304f630f5 -->
##########
superset-frontend/src/components/Menu/index.tsx:
##########
@@ -154,47 +150,27 @@ const StyledSubMenu = styled(AntdMenu.SubMenu)`
opacity: 0;
transform: translateX(-50%);
transition: all ${({ theme }) => theme.transitionTiming}s;
- background-color: ${({ theme }) => theme.colors.primary.base};
}
}
- .ant-menu-submenu-arrow {
- top: 67%;
- }
- & > .ant-menu-submenu-title {
- padding: 0 ${({ theme }) => theme.gridUnit * 6}px 0
- ${({ theme }) => theme.gridUnit * 3}px !important;
- span[role='img'] {
- position: absolute;
- right: ${({ theme }) => -theme.gridUnit + -2}px;
- top: ${({ theme }) => theme.gridUnit * 5.25}px;
- svg {
- font-size: ${({ theme }) => theme.gridUnit * 6}px;
- color: ${({ theme }) => theme.colors.grayscale.base};
- }
- }
- & > span {
- position: relative;
- top: 7px;
- }
- &:hover {
- color: ${({ theme }) => theme.colors.primary.base};
- }
+
+ .ant-dropdown-menu-submenu-arrow:before,
+ .ant-dropdown-menu-submenu-arrow:after {
Review Comment:
### Outdated Submenu Arrow Selectors <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Obsolete submenu arrow selectors using old Ant Design v4 class names, which
won't work with v5.
###### Why this matters
The submenu arrows will not be properly styled or hidden as intended because
the selectors don't match Ant Design v5's class structure.
###### Suggested change ∙ *Feature Preview*
Update the selectors to use the antd5 prefix:
```typescript
.antd5-dropdown-menu-submenu-arrow:before,
.antd5-dropdown-menu-submenu-arrow:after {
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:1c986722-6c1d-47ed-befc-10e2a9322f3c -->
##########
superset-frontend/src/components/Menu/Menu.stories.tsx:
##########
@@ -0,0 +1,63 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Menu, MainNav } from '.';
+
+export default {
+ title: 'Menu',
+ component: Menu as React.FC,
+};
+
+export const MainNavigation = (args: any) => (
+ <MainNav mode="horizontal" {...args}>
+ <Menu.Item>
+ <a href="/">Dashboards</a>
+ </Menu.Item>
+ <Menu.Item>
+ <a href="/">Charts</a>
+ </Menu.Item>
+ <Menu.Item>
+ <a href="/">Datasets</a>
+ </Menu.Item>
+ </MainNav>
+);
+
+export const InteractiveMenu = (args: any) => (
+ <Menu {...args}>
+ <Menu.Item>Dashboards</Menu.Item>
+ <Menu.Item>Charts</Menu.Item>
+ <Menu.Item>Datasets</Menu.Item>
+ </Menu>
+);
+
+InteractiveMenu.args = {
+ defaultSelectedKeys: ['1'],
Review Comment:
### Missing Menu Item Keys <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The defaultSelectedKeys property is set to ['1'] but no corresponding key is
assigned to any Menu.Item component.
###### Why this matters
The menu selection will not work as expected since the items don't have
matching keys for the defaultSelectedKeys value.
###### Suggested change ∙ *Feature Preview*
Add keys to Menu.Item components and update defaultSelectedKeys accordingly:
```tsx
export const InteractiveMenu = (args: any) => (
<Menu {...args}>
<Menu.Item key="1">Dashboards</Menu.Item>
<Menu.Item key="2">Charts</Menu.Item>
<Menu.Item key="3">Datasets</Menu.Item>
</Menu>
);
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:2e7a856d-9229-42be-8ace-bb288b39e885 -->
##########
superset-frontend/src/features/home/SubMenu.tsx:
##########
@@ -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>
Review Comment:
### Incorrect Menu Overflow Prop Name <sub></sub>
<details>
<summary>Tell me more</summary>
###### 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:
```tsx
<Menu mode={showMenu} overflowedIndicator={null}>
{/* ... */}
</Menu>
<Menu mode="horizontal" triggerSubMenuAction="click"
overflowedIndicator={null}>
{/* ... */}
</Menu>
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:67c566f4-22e8-464e-a6bf-766cce2b0f22 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
