You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "geido (via GitHub)" <gi...@apache.org> on 2025/09/12 16:42:21 UTC
[PR] fix: Cosmetic issues [superset]
geido opened a new pull request, #35122:
URL: https://github.com/apache/superset/pull/35122
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
TBD
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Cosmetic issues [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #35122:
URL: https://github.com/apache/superset/pull/35122
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Cosmetic issues [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35122:
URL: https://github.com/apache/superset/pull/35122#discussion_r2359629486
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -85,7 +85,7 @@ const ChartHeaderStyles = styled.div`
& > .header-title {
overflow: hidden;
text-overflow: ellipsis;
- max-width: 100%;
+ max-width: calc(100% - ${theme.sizeUnit * 4}px);
Review Comment:
### Inefficient CSS calc with runtime multiplication <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
CSS calc() function is being executed on every render with a JavaScript expression that performs multiplication.
###### Why this matters
This causes unnecessary recalculation of the CSS value on each component render, potentially impacting rendering performance when the component updates frequently.
###### Suggested change ā *Feature Preview*
Pre-calculate the value outside the styled component or use CSS custom properties to avoid runtime calculations:
```typescript
const headerPadding = theme.sizeUnit * 4;
// Then use: max-width: calc(100% - ${headerPadding}px);
```
Or move the calculation to a CSS variable.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:88d52e59-316e-492d-ba01-57ab9aa44d7d -->
[](88d52e59-316e-492d-ba01-57ab9aa44d7d)
##########
superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:
##########
@@ -191,79 +172,91 @@ export const useDrillDetailMenuItems = ({
drillByDisabled = DISABLED_REASONS.NOT_SUPPORTED;
}
- const drillToDetailMenuItem: MenuItem = drillDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL}
- <MenuItemTooltip title={drillDisabled} />
- </>,
- 'drill-to-detail-disabled',
- props,
- )
+ const drillToDetailMenuItem: ItemType = drillDisabled
+ ? {
+ key: 'drill-to-detail-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL}
+ <MenuItemTooltip title={drillDisabled} />
+ </div>
+ ),
+ ...props,
+ }
: {
key: 'drill-to-detail',
- label: DRILL_TO_DETAIL,
onClick: openModal.bind(null, []),
- ...props,
+ label: DRILL_TO_DETAIL,
};
- const getMenuItemWithTruncation = useMenuItemWithTruncation();
-
- const drillToDetailByMenuItem: MenuItem = drillByDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL_BY}
- <MenuItemTooltip title={drillByDisabled} />
- </>,
- 'drill-to-detail-by-disabled',
- props,
- )
- : {
- key: key || 'drill-to-detail-by',
- label: DRILL_TO_DETAIL_BY,
- children: [
- ...filters.map((filter, i) => ({
- key: `drill-detail-filter-${i}`,
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`,
- onClick: openModal.bind(null, [filter]),
+ const drillToDetailByMenuItem: ItemType | null = !isContextMenu
+ ? null
+ : drillByDisabled
+ ? {
+ key: 'drill-to-detail-by-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}
+ <MenuItemTooltip title={drillByDisabled} />
+ </div>
+ ),
+ ...props,
+ }
+ : {
+ key: key || 'drill-to-detail-by',
+ label: DRILL_TO_DETAIL_BY,
+ popupOffset: [0, submenuYOffset],
+ popupClassName: 'chart-context-submenu',
+ children: [
+ ...filters.map((filter, i) => ({
key: `drill-detail-filter-${i}`,
- children: (
- <>
+ onClick: openModal.bind(null, [filter]),
+ label: (
+ <TruncatedMenuLabel
+ tooltipText={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ aria-label={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ >
{`${DRILL_TO_DETAIL_BY} `}
<StyledFilter stripHTML>{filter.formattedVal}</StyledFilter>
- </>
- ),
- }),
- })),
- filters.length > 1 && {
- key: 'drill-detail-filter-all',
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${t('all')}`,
- onClick: openModal.bind(null, filters),
- key: 'drill-detail-filter-all',
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML={false}>{t('all')}</StyledFilter>
- </>
+ </TruncatedMenuLabel>
),
- }),
- },
- ].filter(Boolean) as MenuItem[],
- onClick: openModal.bind(null, filters),
- forceSubmenuRender: true,
- popupOffset: [0, submenuYOffset],
- popupClassName: 'chart-context-submenu',
- ...props,
- };
- if (isContextMenu) {
- return {
- drillToDetailMenuItem,
- drillToDetailByMenuItem,
- };
+ })),
+ ...(filters.length > 1
+ ? [
+ {
+ key: 'drill-detail-filter-all',
+ onClick: openModal.bind(null, filters),
Review Comment:
### Repeated function binding for all filters handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using Function.prototype.bind() creates a new bound function on every render for the 'all filters' menu item.
###### Why this matters
This prevents React from optimizing re-renders and creates unnecessary function allocations on each render cycle.
###### Suggested change ā *Feature Preview*
Use useCallback to memoize the handler:
```typescript
const handleAllFiltersClick = useCallback((event) => {
openModal(filters, event);
}, [openModal, filters]);
// Then use:
onClick: handleAllFiltersClick
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f5d3627f-4238-49cd-a437-8711fffea837 -->
[](f5d3627f-4238-49cd-a437-8711fffea837)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
+ const theme = useTheme();
+ const [searchInput, setSearchInput] = useState('');
+ const [debouncedSearchInput, setDebouncedSearchInput] = useState('');
+ const [popoverOpen, setPopoverOpen] = useState(false);
+ const ref = useRef<InputRef>(null);
+ const menuItemRef = useRef<HTMLDivElement>(null);
+
+ const columns = useMemo(
+ () => (dataset ? ensureIsArray(dataset.drillable_columns) : []),
+ [dataset],
+ );
+ const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD;
+
+ const handleSelection = useCallback(
+ (event, column) => {
+ onClick(event);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
Review Comment:
### Uncleaned setTimeout in useEffect <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The setTimeout is not cleaned up if the component unmounts or popoverOpen changes before the timeout executes.
###### Why this matters
This can cause the focus operation to execute on an unmounted component, potentially leading to memory leaks and React warnings about setting state on unmounted components.
###### Suggested change ā *Feature Preview*
Store the timeout ID and clear it in the cleanup function:
```typescript
useEffect(() => {
if (popoverOpen) {
const timeoutId = setTimeout(() => {
ref.current?.input?.focus({ preventScroll: true });
}, 100);
return () => clearTimeout(timeoutId);
} else {
setSearchInput('');
setDebouncedSearchInput('');
}
}, [popoverOpen]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b0bd11c6-c6a9-4903-b059-ccab71f927d0 -->
[](b0bd11c6-c6a9-4903-b059-ccab71f927d0)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
Review Comment:
### Loose Type Definitions <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The interface uses loose typing with 'any' in onSelection callback and extends formData with an index signature allowing any additional properties.
###### Why this matters
Loose typing reduces type safety and makes the component's API less predictable, potentially leading to runtime errors that could have been caught during compilation.
###### Suggested change ā *Feature Preview*
Define strict types for the callbacks and formData:
```typescript
export interface DrillBySubmenuProps {
drillByConfig?: ContextMenuFilters['drillBy'];
formData: BaseFormData & {
viz_type: string;
matrixify_enable_vertical_layout?: boolean;
matrixify_enable_horizontal_layout?: boolean;
};
onSelection?: (column: Column, config: ContextMenuFilters['drillBy']) => void;
onClick?: (event: MouseEvent) => void;
...
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e1e0d972-a037-45fd-99f3-d350541d9d6c -->
[](e1e0d972-a037-45fd-99f3-d350541d9d6c)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
+ const theme = useTheme();
+ const [searchInput, setSearchInput] = useState('');
+ const [debouncedSearchInput, setDebouncedSearchInput] = useState('');
+ const [popoverOpen, setPopoverOpen] = useState(false);
+ const ref = useRef<InputRef>(null);
+ const menuItemRef = useRef<HTMLDivElement>(null);
+
+ const columns = useMemo(
+ () => (dataset ? ensureIsArray(dataset.drillable_columns) : []),
+ [dataset],
+ );
+ const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD;
+
+ const handleSelection = useCallback(
+ (event, column) => {
+ onClick(event);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
Review Comment:
### Missing setTimeout cleanup in useEffect <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The setTimeout is not cleaned up when the component unmounts or when popoverOpen changes before the timeout executes, potentially causing focus to be called on an unmounted component.
###### Why this matters
This could lead to memory leaks and React warnings about setting state on unmounted components, especially if the user quickly opens and closes the popover.
###### Suggested change ā *Feature Preview*
Store the timeout ID and clear it in the cleanup function:
```typescript
useEffect(() => {
let timeoutId: NodeJS.Timeout;
if (popoverOpen) {
timeoutId = setTimeout(() => {
ref.current?.input?.focus({ preventScroll: true });
}, 100);
} else {
setSearchInput('');
setDebouncedSearchInput('');
}
return () => {
if (timeoutId) {
clearTimeout(timeoutId);
}
};
}, [popoverOpen]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f7f14a59-e943-4db8-8cf4-2270bab6c412 -->
[](f7f14a59-e943-4db8-8cf4-2270bab6c412)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
+ const theme = useTheme();
+ const [searchInput, setSearchInput] = useState('');
+ const [debouncedSearchInput, setDebouncedSearchInput] = useState('');
+ const [popoverOpen, setPopoverOpen] = useState(false);
+ const ref = useRef<InputRef>(null);
+ const menuItemRef = useRef<HTMLDivElement>(null);
+
+ const columns = useMemo(
+ () => (dataset ? ensureIsArray(dataset.drillable_columns) : []),
+ [dataset],
+ );
+ const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD;
+
+ const handleSelection = useCallback(
+ (event, column) => {
+ onClick(event);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
+ } else {
+ // Reset search input when menu is closed
+ setSearchInput('');
+ setDebouncedSearchInput('');
+ }
+ }, [popoverOpen]);
+
+ const hasDrillBy = drillByConfig?.groupbyFieldName;
+
+ const handlesDimensionContextMenu = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.behaviors.find(behavior => behavior === Behavior.DrillBy),
+ [formData.viz_type],
+ );
+
+ const debouncedSetSearchInput = useMemo(
+ () =>
+ debounce((value: string) => {
+ setDebouncedSearchInput(value);
+ }, Constants.FAST_DEBOUNCE),
+ [],
+ );
+
+ const handleInput = (value: string) => {
+ setSearchInput(value);
+ debouncedSetSearchInput(value);
+ };
+
+ const filteredColumns = useMemo(
+ () =>
+ columns
+ .filter(column => {
+ // Filter out excluded columns
+ const excludedColumnNames =
+ excludedColumns?.map(col => col.column_name) || [];
+ return !excludedColumnNames.includes(column.column_name);
+ })
+ .filter(column =>
+ (column.verbose_name || column.column_name)
+ .toLowerCase()
+ .includes(debouncedSearchInput.toLowerCase()),
+ ),
+ [columns, debouncedSearchInput, excludedColumns],
+ );
Review Comment:
### Inefficient excluded columns filtering <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The excludedColumnNames array is recreated on every filter iteration, causing O(nĀ²) time complexity for the exclusion check.
###### Why this matters
For large datasets, this creates unnecessary computational overhead as the map operation and array creation happens for each column being filtered.
###### Suggested change ā *Feature Preview*
Pre-compute the excluded column names outside the filter:
```typescript
const filteredColumns = useMemo(() => {
const excludedColumnNames = new Set(
excludedColumns?.map(col => col.column_name) || []
);
return columns
.filter(column => !excludedColumnNames.has(column.column_name))
.filter(column =>
(column.verbose_name || column.column_name)
.toLowerCase()
.includes(debouncedSearchInput.toLowerCase()),
);
}, [columns, debouncedSearchInput, excludedColumns]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2aa86a34-9031-4883-8031-3ddee6f9c226 -->
[](2aa86a34-9031-4883-8031-3ddee6f9c226)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
+ const theme = useTheme();
+ const [searchInput, setSearchInput] = useState('');
+ const [debouncedSearchInput, setDebouncedSearchInput] = useState('');
+ const [popoverOpen, setPopoverOpen] = useState(false);
+ const ref = useRef<InputRef>(null);
+ const menuItemRef = useRef<HTMLDivElement>(null);
+
+ const columns = useMemo(
+ () => (dataset ? ensureIsArray(dataset.drillable_columns) : []),
+ [dataset],
+ );
+ const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD;
+
+ const handleSelection = useCallback(
+ (event, column) => {
+ onClick(event);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
Review Comment:
### Event type mismatch in handleSelection callback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The handleSelection callback receives an event parameter but the onClick prop expects a MouseEvent type, while the actual event passed could be a React SyntheticEvent from the VirtualizedMenuItem click handler.
###### Why this matters
This type mismatch could cause runtime errors or unexpected behavior when the onClick handler tries to access MouseEvent-specific properties that don't exist on SyntheticEvent objects.
###### Suggested change ā *Feature Preview*
Update the onClick prop type to accept React's SyntheticEvent or cast the event parameter:
```typescript
// Option 1: Update the prop type
onClick?: (event: React.MouseEvent | MouseEvent) => void;
// Option 2: Cast the event in handleSelection
const handleSelection = useCallback(
(event, column) => {
onClick(event as MouseEvent);
// ... rest of the function
},
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:db5331ff-4891-45d4-8c08-7179f009184b -->
[](db5331ff-4891-45d4-8c08-7179f009184b)
##########
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx:
##########
@@ -264,115 +257,76 @@ const ChartContextMenu = (
itemsCount = 1; // "No actions" appears if no actions in menu
}
+ const drillDetailMenuItems = useDrillDetailMenuItems({
+ formData: drillFormData,
+ filters: filters?.drillToDetail,
+ setFilters,
+ isContextMenu: true,
+ contextMenuY: clientY,
+ onSelection,
+ submenuIndex: showCrossFilters ? 2 : 1,
+ setShowModal: setDrillModalIsOpen,
+ dataset: filteredDataset,
+ isLoadingDataset,
+ ...(additionalConfig?.drillToDetail || {}),
+ });
Review Comment:
### Unconditional hook execution for drill detail menu items <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useDrillDetailMenuItems hook is called unconditionally on every render, even when drill-to-detail functionality is not shown or needed.
###### Why this matters
This causes unnecessary computation and potential API calls or expensive operations within the hook, degrading performance especially when the context menu is frequently re-rendered or when drill-to-detail is disabled.
###### Suggested change ā *Feature Preview*
Move the hook call inside a conditional block or use useMemo with proper dependencies to only execute when showDrillToDetail is true:
```typescript
const drillDetailMenuItems = useMemo(() => {
if (!showDrillToDetail) return [];
return useDrillDetailMenuItems({
formData: drillFormData,
filters: filters?.drillToDetail,
setFilters,
isContextMenu: true,
contextMenuY: clientY,
onSelection,
submenuIndex: showCrossFilters ? 2 : 1,
setShowModal: setDrillModalIsOpen,
dataset: filteredDataset,
isLoadingDataset,
...(additionalConfig?.drillToDetail || {}),
});
}, [showDrillToDetail, drillFormData, filters?.drillToDetail, /* other deps */]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9e28002a-47d4-495c-8a8c-18e452e6b137 -->
[](9e28002a-47d4-495c-8a8c-18e452e6b137)
##########
superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:
##########
@@ -191,79 +172,91 @@ export const useDrillDetailMenuItems = ({
drillByDisabled = DISABLED_REASONS.NOT_SUPPORTED;
}
- const drillToDetailMenuItem: MenuItem = drillDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL}
- <MenuItemTooltip title={drillDisabled} />
- </>,
- 'drill-to-detail-disabled',
- props,
- )
+ const drillToDetailMenuItem: ItemType = drillDisabled
+ ? {
+ key: 'drill-to-detail-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL}
+ <MenuItemTooltip title={drillDisabled} />
+ </div>
+ ),
+ ...props,
+ }
: {
key: 'drill-to-detail',
- label: DRILL_TO_DETAIL,
onClick: openModal.bind(null, []),
- ...props,
+ label: DRILL_TO_DETAIL,
};
Review Comment:
### Duplicated menu item creation logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Menu item creation logic is duplicated and mixed with presentation concerns, making it difficult to maintain consistent styling and behavior across menu items.
###### Why this matters
Violates DRY principle and mixes concerns, making it harder to maintain consistent menu item styling and behavior. Changes to menu item structure require updates in multiple places.
###### Suggested change ā *Feature Preview*
Extract menu item creation into a reusable factory function:
```typescript
const createMenuItem = ({
key,
label,
disabled,
onClick,
tooltipTitle,
...rest
}: MenuItemConfig): ItemType => ({
key,
label: disabled ? (
<DisabledMenuItemLabel label={label} tooltipTitle={tooltipTitle} />
) : label,
disabled,
onClick,
...rest
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1dbac2a1-e4ce-43ea-8a25-5d789f0265e6 -->
[](1dbac2a1-e4ce-43ea-8a25-5d789f0265e6)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Cosmetic issues [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #35122:
URL: https://github.com/apache/superset/pull/35122#discussion_r2363395362
##########
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx:
##########
@@ -264,115 +257,76 @@ const ChartContextMenu = (
itemsCount = 1; // "No actions" appears if no actions in menu
}
+ const drillDetailMenuItems = useDrillDetailMenuItems({
+ formData: drillFormData,
+ filters: filters?.drillToDetail,
+ setFilters,
+ isContextMenu: true,
+ contextMenuY: clientY,
+ onSelection,
+ submenuIndex: showCrossFilters ? 2 : 1,
+ setShowModal: setDrillModalIsOpen,
+ dataset: filteredDataset,
+ isLoadingDataset,
+ ...(additionalConfig?.drillToDetail || {}),
+ });
+
if (showCrossFilters) {
const isCrossFilterDisabled =
!isCrossFilteringSupportedByChart ||
!crossFiltersEnabled ||
!filters?.crossFilter;
- let crossFilteringTooltipTitle: ReactNode = null;
- if (!isCrossFilterDisabled) {
- crossFilteringTooltipTitle = (
- <>
- <div>
- {t(
- 'Cross-filter will be applied to all of the charts that use this dataset.',
Review Comment:
Good catch!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Cosmetic issues [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #35122:
URL: https://github.com/apache/superset/pull/35122#issuecomment-3286024752
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](https://app.korbit.ai/settings)
[](53557ec8-fee7-40b0-8410-6fbede90b9d9)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Cosmetic issues [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35122:
URL: https://github.com/apache/superset/pull/35122#discussion_r2363469150
##########
superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:
##########
@@ -191,79 +172,110 @@ export const useDrillDetailMenuItems = ({
drillByDisabled = DISABLED_REASONS.NOT_SUPPORTED;
}
- const drillToDetailMenuItem: MenuItem = drillDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL}
- <MenuItemTooltip title={drillDisabled} />
- </>,
- 'drill-to-detail-disabled',
- props,
- )
+ const drillToDetailMenuItem: ItemType = drillDisabled
+ ? {
+ key: 'drill-to-detail-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL}
+ <MenuItemTooltip title={drillDisabled} />
+ </div>
+ ),
+ ...props,
+ }
: {
key: 'drill-to-detail',
- label: DRILL_TO_DETAIL,
onClick: openModal.bind(null, []),
- ...props,
+ label: DRILL_TO_DETAIL,
};
- const getMenuItemWithTruncation = useMenuItemWithTruncation();
-
- const drillToDetailByMenuItem: MenuItem = drillByDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL_BY}
- <MenuItemTooltip title={drillByDisabled} />
- </>,
- 'drill-to-detail-by-disabled',
- props,
- )
- : {
- key: key || 'drill-to-detail-by',
- label: DRILL_TO_DETAIL_BY,
- children: [
- ...filters.map((filter, i) => ({
- key: `drill-detail-filter-${i}`,
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`,
- onClick: openModal.bind(null, [filter]),
+ const drillToDetailByMenuItem: ItemType | null = !isContextMenu
+ ? null
+ : drillByDisabled
+ ? {
+ key: 'drill-to-detail-by-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}
+ <MenuItemTooltip title={drillByDisabled} />
+ </div>
+ ),
+ ...props,
+ }
+ : {
+ key: key || 'drill-to-detail-by',
+ label: DRILL_TO_DETAIL_BY,
+ popupOffset: [0, submenuYOffset],
+ popupClassName: 'chart-context-submenu',
+ children: [
+ ...filters.map((filter, i) => ({
key: `drill-detail-filter-${i}`,
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML>{filter.formattedVal}</StyledFilter>
- </>
- ),
- }),
- })),
- filters.length > 1 && {
- key: 'drill-detail-filter-all',
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${t('all')}`,
- onClick: openModal.bind(null, filters),
- key: 'drill-detail-filter-all',
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML={false}>{t('all')}</StyledFilter>
- </>
+ onClick: openModal.bind(null, [filter]),
+ label: (
+ <div
+ css={css`
+ max-width: 200px;
+ `}
+ >
+ <TruncatedMenuLabel
+ tooltipText={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ aria-label={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ >
+ <span
+ css={css`
+ display: inline;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}{' '}
+ <StyledFilter stripHTML>
+ {filter.formattedVal}
+ </StyledFilter>
+ </span>
+ </TruncatedMenuLabel>
+ </div>
),
- }),
- },
- ].filter(Boolean) as MenuItem[],
- onClick: openModal.bind(null, filters),
- forceSubmenuRender: true,
- popupOffset: [0, submenuYOffset],
- popupClassName: 'chart-context-submenu',
- ...props,
- };
- if (isContextMenu) {
- return {
- drillToDetailMenuItem,
- drillToDetailByMenuItem,
- };
+ })),
Review Comment:
### Unnecessary object recreation in filters.map <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The filters.map() creates a new menu item object for each filter on every render, causing unnecessary re-renders and object allocations.
###### Why this matters
This leads to performance degradation when there are many filters, as React will treat each menu item as a new object and re-render the entire submenu unnecessarily.
###### Suggested change ā *Feature Preview*
Memoize the filter menu items using useMemo with filters as dependency, or extract the mapping function to avoid inline object creation:
```typescript
const filterMenuItems = useMemo(
() => filters.map((filter, i) => ({
key: `drill-detail-filter-${i}`,
onClick: openModal.bind(null, [filter]),
label: (
// ... existing label JSX
),
})),
[filters, openModal]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5fd1575c-5d2f-4a19-9915-da1970ab258e -->
[](5fd1575c-5d2f-4a19-9915-da1970ab258e)
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -85,7 +85,7 @@ const ChartHeaderStyles = styled.div`
& > .header-title {
overflow: hidden;
text-overflow: ellipsis;
- max-width: 100%;
+ max-width: calc(100% - ${theme.sizeUnit * 4}px);
Review Comment:
### Hardcoded header controls width assumption <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The hardcoded subtraction of `theme.sizeUnit * 4` pixels may not accurately reflect the actual width of the header controls, potentially causing text truncation or overflow issues.
###### Why this matters
If the actual width of the header controls differs from the hardcoded 4 size units, the title text may be unnecessarily truncated when controls are narrower, or may overflow and overlap with controls when they are wider.
###### Suggested change ā *Feature Preview*
Calculate the actual width of the header controls dynamically using a ref and ResizeObserver, or use CSS flexbox properties to let the browser handle the space distribution automatically:
```css
& > .header-title {
overflow: hidden;
text-overflow: ellipsis;
flex: 1 1 0;
min-width: 0;
display: -webkit-box;
-webkit-line-clamp: 2;
-webkit-box-orient: vertical;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6491489f-1d9b-453d-b199-a75f5822630d -->
[](6491489f-1d9b-453d-b199-a75f5822630d)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,342 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
Review Comment:
### Component has too many responsibilities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component is handling too many responsibilities including search, filtering, UI state management, and event handling, violating the Single Responsibility Principle.
###### Why this matters
Large components with multiple responsibilities are harder to maintain, test, and reuse. This can lead to increased complexity and technical debt over time.
###### Suggested change ā *Feature Preview*
Extract functionality into smaller, focused components:
```typescript
const SearchInput = ({ onSearch, showSearch, ...props }) => { /* ... */ };
const ColumnsList = ({ columns, onSelect }) => { /* ... */ };
const DrillByMenu = ({ isDisabled, tooltip, ...props }) => { /* ... */ };
export const DrillBySubmenu = (props: DrillBySubmenuProps) => {
// Core logic for coordinating between components
return (
<DrillByMenu {...menuProps}>
<SearchInput {...searchProps} />
<ColumnsList {...columnsProps} />
</DrillByMenu>
);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ac76972c-4f5b-486b-9665-d44026473b00 -->
[](ac76972c-4f5b-486b-9665-d44026473b00)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,342 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
+ const theme = useTheme();
+ const [searchInput, setSearchInput] = useState('');
+ const [debouncedSearchInput, setDebouncedSearchInput] = useState('');
+ const [popoverOpen, setPopoverOpen] = useState(false);
+ const ref = useRef<InputRef>(null);
+ const menuItemRef = useRef<HTMLDivElement>(null);
+
+ const columns = useMemo(
+ () => (dataset ? ensureIsArray(dataset.drillable_columns) : []),
+ [dataset],
+ );
+ const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD;
+
+ const handleSelection = useCallback(
+ (event, column) => {
+ onClick(event as MouseEvent);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ let timeoutId: NodeJS.Timeout;
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ timeoutId = setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
+ } else {
+ // Reset search input when menu is closed
+ setSearchInput('');
+ setDebouncedSearchInput('');
+ }
+ return () => {
+ if (timeoutId) clearTimeout(timeoutId);
+ };
+ }, [popoverOpen]);
+
+ const hasDrillBy = drillByConfig?.groupbyFieldName;
+
+ const handlesDimensionContextMenu = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.behaviors.find(behavior => behavior === Behavior.DrillBy),
+ [formData.viz_type],
+ );
+
+ const debouncedSetSearchInput = useMemo(
+ () =>
+ debounce((value: string) => {
+ setDebouncedSearchInput(value);
+ }, Constants.FAST_DEBOUNCE),
+ [],
+ );
Review Comment:
### Uncancelled debounce on unmount <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Debounced search handler is created with lodash debounce but there is no cleanup on unmount.
###### Why this matters
If the component unmounts while a debounced timer is pending, the timer may fire and call state updates on an unmounted component, potentially leading to memory leaks or React state warnings.
###### Suggested change ā *Feature Preview*
Add a cleanup effect to cancel the debounced function when the component unmounts, for example:
```tsx
useEffect(() => {
return () => {
(debouncedSetSearchInput as any).cancel?.();
};
}, [debouncedSetSearchInput]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4e038187-3a3c-4f54-9885-a9aedd13319a -->
[](4e038187-3a3c-4f54-9885-a9aedd13319a)
##########
superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:
##########
@@ -191,79 +172,110 @@ export const useDrillDetailMenuItems = ({
drillByDisabled = DISABLED_REASONS.NOT_SUPPORTED;
}
- const drillToDetailMenuItem: MenuItem = drillDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL}
- <MenuItemTooltip title={drillDisabled} />
- </>,
- 'drill-to-detail-disabled',
- props,
- )
+ const drillToDetailMenuItem: ItemType = drillDisabled
+ ? {
+ key: 'drill-to-detail-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL}
+ <MenuItemTooltip title={drillDisabled} />
+ </div>
+ ),
+ ...props,
+ }
: {
key: 'drill-to-detail',
- label: DRILL_TO_DETAIL,
onClick: openModal.bind(null, []),
- ...props,
+ label: DRILL_TO_DETAIL,
};
- const getMenuItemWithTruncation = useMenuItemWithTruncation();
-
- const drillToDetailByMenuItem: MenuItem = drillByDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL_BY}
- <MenuItemTooltip title={drillByDisabled} />
- </>,
- 'drill-to-detail-by-disabled',
- props,
- )
- : {
- key: key || 'drill-to-detail-by',
- label: DRILL_TO_DETAIL_BY,
- children: [
- ...filters.map((filter, i) => ({
- key: `drill-detail-filter-${i}`,
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`,
- onClick: openModal.bind(null, [filter]),
+ const drillToDetailByMenuItem: ItemType | null = !isContextMenu
+ ? null
+ : drillByDisabled
+ ? {
+ key: 'drill-to-detail-by-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}
+ <MenuItemTooltip title={drillByDisabled} />
+ </div>
+ ),
+ ...props,
+ }
+ : {
+ key: key || 'drill-to-detail-by',
+ label: DRILL_TO_DETAIL_BY,
+ popupOffset: [0, submenuYOffset],
+ popupClassName: 'chart-context-submenu',
+ children: [
+ ...filters.map((filter, i) => ({
key: `drill-detail-filter-${i}`,
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML>{filter.formattedVal}</StyledFilter>
- </>
- ),
- }),
- })),
- filters.length > 1 && {
- key: 'drill-detail-filter-all',
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${t('all')}`,
- onClick: openModal.bind(null, filters),
- key: 'drill-detail-filter-all',
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML={false}>{t('all')}</StyledFilter>
- </>
+ onClick: openModal.bind(null, [filter]),
+ label: (
+ <div
+ css={css`
+ max-width: 200px;
+ `}
+ >
+ <TruncatedMenuLabel
+ tooltipText={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ aria-label={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ >
+ <span
+ css={css`
+ display: inline;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}{' '}
+ <StyledFilter stripHTML>
+ {filter.formattedVal}
+ </StyledFilter>
+ </span>
+ </TruncatedMenuLabel>
+ </div>
),
- }),
- },
- ].filter(Boolean) as MenuItem[],
- onClick: openModal.bind(null, filters),
- forceSubmenuRender: true,
- popupOffset: [0, submenuYOffset],
- popupClassName: 'chart-context-submenu',
- ...props,
- };
- if (isContextMenu) {
- return {
- drillToDetailMenuItem,
- drillToDetailByMenuItem,
- };
+ })),
+ ...(filters.length > 1
+ ? [
+ {
+ key: 'drill-detail-filter-all',
+ onClick: openModal.bind(null, filters),
+ label: (
+ <div
+ aria-label={`${DRILL_TO_DETAIL_BY} ${t('all')}`}
+ css={css`
+ max-width: 200px;
+ `}
+ >
+ {`${DRILL_TO_DETAIL_BY} `}
+ <StyledFilter stripHTML={false}>
+ {t('all')}
+ </StyledFilter>
+ </div>
+ ),
+ },
+ ]
+ : []),
+ ],
+ ...props,
+ };
+
+ const menuItems: ItemType[] = [drillToDetailMenuItem];
+ if (drillToDetailByMenuItem) {
+ menuItems.push(drillToDetailByMenuItem);
}
- return {
- drillToDetailMenuItem,
- };
+
+ return menuItems;
Review Comment:
### Array recreation on every render <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating and mutating a new array on every render instead of using a memoized approach.
###### Why this matters
This causes the hook to return a new array reference on every call, triggering unnecessary re-renders in components that depend on this menu items array.
###### Suggested change ā *Feature Preview*
Memoize the final menu items array using useMemo:
```typescript
return useMemo(() => {
const items = [drillToDetailMenuItem];
if (drillToDetailByMenuItem) {
items.push(drillToDetailByMenuItem);
}
return items;
}, [drillToDetailMenuItem, drillToDetailByMenuItem]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6320eb51-7c6b-4787-ae77-bc36ee5675a0 -->
[](6320eb51-7c6b-4787-ae77-bc36ee5675a0)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Cosmetic issues [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #35122:
URL: https://github.com/apache/superset/pull/35122#discussion_r2363398918
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
+ const theme = useTheme();
+ const [searchInput, setSearchInput] = useState('');
+ const [debouncedSearchInput, setDebouncedSearchInput] = useState('');
+ const [popoverOpen, setPopoverOpen] = useState(false);
+ const ref = useRef<InputRef>(null);
+ const menuItemRef = useRef<HTMLDivElement>(null);
+
+ const columns = useMemo(
+ () => (dataset ? ensureIsArray(dataset.drillable_columns) : []),
+ [dataset],
+ );
+ const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD;
+
+ const handleSelection = useCallback(
+ (event, column) => {
+ onClick(event);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
+ } else {
+ // Reset search input when menu is closed
+ setSearchInput('');
+ setDebouncedSearchInput('');
+ }
+ }, [popoverOpen]);
+
+ const hasDrillBy = drillByConfig?.groupbyFieldName;
+
+ const handlesDimensionContextMenu = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.behaviors.find(behavior => behavior === Behavior.DrillBy),
+ [formData.viz_type],
+ );
+
+ const debouncedSetSearchInput = useMemo(
+ () =>
+ debounce((value: string) => {
+ setDebouncedSearchInput(value);
+ }, Constants.FAST_DEBOUNCE),
+ [],
+ );
+
+ const handleInput = (value: string) => {
+ setSearchInput(value);
+ debouncedSetSearchInput(value);
+ };
+
+ const filteredColumns = useMemo(
+ () =>
+ columns
+ .filter(column => {
+ // Filter out excluded columns
+ const excludedColumnNames =
+ excludedColumns?.map(col => col.column_name) || [];
+ return !excludedColumnNames.includes(column.column_name);
+ })
+ .filter(column =>
+ (column.verbose_name || column.column_name)
+ .toLowerCase()
+ .includes(debouncedSearchInput.toLowerCase()),
+ ),
+ [columns, debouncedSearchInput, excludedColumns],
+ );
+
+ let tooltip: ReactNode;
+
+ if (!handlesDimensionContextMenu) {
+ tooltip = t('Drill by is not yet supported for this chart type');
+ } else if (!hasDrillBy) {
+ tooltip = t('Drill by is not available for this data point');
+ }
+
+ if (
+ formData.matrixify_enable_vertical_layout === true ||
+ formData.matrixify_enable_horizontal_layout === true
+ ) {
+ return null;
+ }
+
+ const isDisabled = !handlesDimensionContextMenu || !hasDrillBy;
+
+ const Row = ({
+ index,
+ data,
+ style,
+ }: {
+ index: number;
+ data: { columns: Column[] };
+ style: CSSProperties;
+ }) => {
+ const { columns } = data;
+ const column = columns[index];
+ return (
+ <VirtualizedMenuItem
+ tooltipText={column.verbose_name || column.column_name}
+ onClick={e => handleSelection(e, column)}
+ style={style}
+ >
+ {column.verbose_name || column.column_name}
+ </VirtualizedMenuItem>
+ );
+ };
+
+ const popoverContent = (
+ <div
+ role="menu"
+ tabIndex={0}
+ data-test="drill-by-submenu"
+ css={css`
+ width: 220px;
+ max-width: 220px;
+ .ant-input-affix-wrapper {
+ margin-bottom: ${theme.sizeUnit * 2}px;
+ }
+ `}
+ onClick={e => e.stopPropagation()}
+ >
+ {showSearch && (
+ <Input
+ ref={ref}
+ prefix={
+ <Icons.SearchOutlined iconSize="l" iconColor={theme.colorIcon} />
+ }
+ onChange={e => {
+ e.stopPropagation();
+ handleInput(e.target.value);
+ }}
+ placeholder={t('Search columns')}
+ onClick={e => {
+ // prevent closing menu when clicking on input
+ e.nativeEvent.stopImmediatePropagation();
+ }}
+ allowClear
+ css={css`
+ width: 100%;
+ box-shadow: none;
+ `}
+ value={searchInput}
+ />
+ )}
+ {isLoadingDataset ? (
+ <div
+ css={css`
+ padding: ${theme.sizeUnit * 3}px 0;
+ `}
+ >
+ <Loading position="inline-centered" />
+ </div>
+ ) : filteredColumns.length ? (
+ <List
+ width="100%"
+ height={SUBMENU_HEIGHT}
+ itemSize={35}
+ itemCount={filteredColumns.length}
+ itemData={{ columns: filteredColumns }}
+ overscanCount={20}
+ >
+ {Row}
+ </List>
+ ) : (
+ <div
+ css={css`
+ padding: ${theme.sizeUnit * 2}px;
+ color: ${theme.colorTextDisabled};
+ text-align: center;
+ `}
+ >
+ {t('No columns found')}
+ </div>
+ )}
+ </div>
+ );
+
+ const menuItem = (
+ <div
+ ref={menuItemRef}
+ role="button"
+ tabIndex={isDisabled ? -1 : 0}
+ css={css`
+ display: flex;
+ align-items: center;
+ justify-content: space-between;
+ cursor: ${isDisabled ? 'not-allowed' : 'pointer'};
+ color: ${isDisabled ? theme.colorTextDisabled : 'inherit'};
+ &:hover {
+ background: transparent;
+ }
+ `}
+ onClick={() => !isDisabled && setPopoverOpen(!popoverOpen)}
+ onKeyDown={e => {
+ if (!isDisabled && (e.key === 'Enter' || e.key === ' ')) {
+ e.preventDefault();
+ setPopoverOpen(!popoverOpen);
+ }
+ }}
+ >
+ <span>{t('Drill by')}</span>
+ {isDisabled ? (
+ <MenuItemTooltip title={tooltip} />
+ ) : (
+ <Icons.RightOutlined iconSize="s" iconColor={theme.colorTextTertiary} />
+ )}
+ </div>
+ );
+
+ if (isDisabled) {
+ return menuItem;
+ }
+
+ return (
+ <Popover
+ content={popoverContent}
+ placement="rightTop"
+ open={popoverOpen}
+ onOpenChange={setPopoverOpen}
+ trigger={['hover', 'click']}
+ arrow={false}
+ styles={{
+ root: {
+ paddingLeft: 0,
+ },
+ body: {
+ padding: theme.sizeUnit * 2,
+ boxShadow: theme.boxShadow,
+ borderRadius: theme.borderRadius,
+ },
+ }}
+ {...rest}
+ >
+ {menuItem}
+ </Popover>
Review Comment:
It seems that these menu items are not accessible in the first place as most charts do not react to keyboard events correctly. Definitely something that calls for an accessibility project around viz plugins!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Cosmetic issues [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #35122:
URL: https://github.com/apache/superset/pull/35122#discussion_r2360057486
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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 {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
+ const theme = useTheme();
+ const [searchInput, setSearchInput] = useState('');
+ const [debouncedSearchInput, setDebouncedSearchInput] = useState('');
+ const [popoverOpen, setPopoverOpen] = useState(false);
+ const ref = useRef<InputRef>(null);
+ const menuItemRef = useRef<HTMLDivElement>(null);
+
+ const columns = useMemo(
+ () => (dataset ? ensureIsArray(dataset.drillable_columns) : []),
+ [dataset],
+ );
+ const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD;
+
+ const handleSelection = useCallback(
+ (event, column) => {
+ onClick(event);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
+ } else {
+ // Reset search input when menu is closed
+ setSearchInput('');
+ setDebouncedSearchInput('');
+ }
+ }, [popoverOpen]);
+
+ const hasDrillBy = drillByConfig?.groupbyFieldName;
+
+ const handlesDimensionContextMenu = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.behaviors.find(behavior => behavior === Behavior.DrillBy),
+ [formData.viz_type],
+ );
+
+ const debouncedSetSearchInput = useMemo(
+ () =>
+ debounce((value: string) => {
+ setDebouncedSearchInput(value);
+ }, Constants.FAST_DEBOUNCE),
+ [],
+ );
+
+ const handleInput = (value: string) => {
+ setSearchInput(value);
+ debouncedSetSearchInput(value);
+ };
+
+ const filteredColumns = useMemo(
+ () =>
+ columns
+ .filter(column => {
+ // Filter out excluded columns
+ const excludedColumnNames =
+ excludedColumns?.map(col => col.column_name) || [];
+ return !excludedColumnNames.includes(column.column_name);
+ })
+ .filter(column =>
+ (column.verbose_name || column.column_name)
+ .toLowerCase()
+ .includes(debouncedSearchInput.toLowerCase()),
+ ),
+ [columns, debouncedSearchInput, excludedColumns],
+ );
+
+ let tooltip: ReactNode;
+
+ if (!handlesDimensionContextMenu) {
+ tooltip = t('Drill by is not yet supported for this chart type');
+ } else if (!hasDrillBy) {
+ tooltip = t('Drill by is not available for this data point');
+ }
+
+ if (
+ formData.matrixify_enable_vertical_layout === true ||
+ formData.matrixify_enable_horizontal_layout === true
+ ) {
+ return null;
+ }
+
+ const isDisabled = !handlesDimensionContextMenu || !hasDrillBy;
+
+ const Row = ({
+ index,
+ data,
+ style,
+ }: {
+ index: number;
+ data: { columns: Column[] };
+ style: CSSProperties;
+ }) => {
+ const { columns } = data;
+ const column = columns[index];
+ return (
+ <VirtualizedMenuItem
+ tooltipText={column.verbose_name || column.column_name}
+ onClick={e => handleSelection(e, column)}
+ style={style}
+ >
+ {column.verbose_name || column.column_name}
+ </VirtualizedMenuItem>
+ );
+ };
+
+ const popoverContent = (
+ <div
+ role="menu"
+ tabIndex={0}
+ data-test="drill-by-submenu"
+ css={css`
+ width: 220px;
+ max-width: 220px;
+ .ant-input-affix-wrapper {
+ margin-bottom: ${theme.sizeUnit * 2}px;
+ }
+ `}
+ onClick={e => e.stopPropagation()}
+ >
+ {showSearch && (
+ <Input
+ ref={ref}
+ prefix={
+ <Icons.SearchOutlined iconSize="l" iconColor={theme.colorIcon} />
+ }
+ onChange={e => {
+ e.stopPropagation();
+ handleInput(e.target.value);
+ }}
+ placeholder={t('Search columns')}
+ onClick={e => {
+ // prevent closing menu when clicking on input
+ e.nativeEvent.stopImmediatePropagation();
+ }}
+ allowClear
+ css={css`
+ width: 100%;
+ box-shadow: none;
+ `}
+ value={searchInput}
+ />
+ )}
+ {isLoadingDataset ? (
+ <div
+ css={css`
+ padding: ${theme.sizeUnit * 3}px 0;
+ `}
+ >
+ <Loading position="inline-centered" />
+ </div>
+ ) : filteredColumns.length ? (
+ <List
+ width="100%"
+ height={SUBMENU_HEIGHT}
+ itemSize={35}
+ itemCount={filteredColumns.length}
+ itemData={{ columns: filteredColumns }}
+ overscanCount={20}
+ >
+ {Row}
+ </List>
+ ) : (
+ <div
+ css={css`
+ padding: ${theme.sizeUnit * 2}px;
+ color: ${theme.colorTextDisabled};
+ text-align: center;
+ `}
+ >
+ {t('No columns found')}
+ </div>
+ )}
+ </div>
+ );
+
+ const menuItem = (
+ <div
+ ref={menuItemRef}
+ role="button"
+ tabIndex={isDisabled ? -1 : 0}
+ css={css`
+ display: flex;
+ align-items: center;
+ justify-content: space-between;
+ cursor: ${isDisabled ? 'not-allowed' : 'pointer'};
+ color: ${isDisabled ? theme.colorTextDisabled : 'inherit'};
+ &:hover {
+ background: transparent;
+ }
+ `}
+ onClick={() => !isDisabled && setPopoverOpen(!popoverOpen)}
+ onKeyDown={e => {
+ if (!isDisabled && (e.key === 'Enter' || e.key === ' ')) {
+ e.preventDefault();
+ setPopoverOpen(!popoverOpen);
+ }
+ }}
+ >
+ <span>{t('Drill by')}</span>
+ {isDisabled ? (
+ <MenuItemTooltip title={tooltip} />
+ ) : (
+ <Icons.RightOutlined iconSize="s" iconColor={theme.colorTextTertiary} />
+ )}
+ </div>
+ );
+
+ if (isDisabled) {
+ return menuItem;
+ }
+
+ return (
+ <Popover
+ content={popoverContent}
+ placement="rightTop"
+ open={popoverOpen}
+ onOpenChange={setPopoverOpen}
+ trigger={['hover', 'click']}
+ arrow={false}
+ styles={{
+ root: {
+ paddingLeft: 0,
+ },
+ body: {
+ padding: theme.sizeUnit * 2,
+ boxShadow: theme.boxShadow,
+ borderRadius: theme.borderRadius,
+ },
+ }}
+ {...rest}
+ >
+ {menuItem}
+ </Popover>
Review Comment:
Oh, you just don't use the submenu to fix the virtualization. Do the accessibility and keyboard navigation work like this?
##########
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx:
##########
@@ -264,115 +257,76 @@ const ChartContextMenu = (
itemsCount = 1; // "No actions" appears if no actions in menu
}
+ const drillDetailMenuItems = useDrillDetailMenuItems({
+ formData: drillFormData,
+ filters: filters?.drillToDetail,
+ setFilters,
+ isContextMenu: true,
+ contextMenuY: clientY,
+ onSelection,
+ submenuIndex: showCrossFilters ? 2 : 1,
+ setShowModal: setDrillModalIsOpen,
+ dataset: filteredDataset,
+ isLoadingDataset,
+ ...(additionalConfig?.drillToDetail || {}),
+ });
+
if (showCrossFilters) {
const isCrossFilterDisabled =
!isCrossFilteringSupportedByChart ||
!crossFiltersEnabled ||
!filters?.crossFilter;
- let crossFilteringTooltipTitle: ReactNode = null;
- if (!isCrossFilterDisabled) {
- crossFilteringTooltipTitle = (
- <>
- <div>
- {t(
- 'Cross-filter will be applied to all of the charts that use this dataset.',
Review Comment:
We don't use the tooltips here anymore?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org