You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/10/07 07:09:14 UTC
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21351: feat(dashboard): menu improvements, fallback support for Drill to Detail
zhaoyongjie commented on code in PR #21351:
URL: https://github.com/apache/superset/pull/21351#discussion_r989712522
##########
superset-frontend/src/types/ChartSource.ts:
##########
@@ -0,0 +1,23 @@
+/**
+ * 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.
+ */
+
+export enum ChartSource {
Review Comment:
one nite, the member of the enum is a constant so should use uppercase naming.
##########
superset-frontend/src/components/Chart/ChartContextMenu.tsx:
##########
@@ -23,90 +23,102 @@ import React, {
useImperativeHandle,
useState,
} from 'react';
-import { QueryObjectFilterClause, t, styled } from '@superset-ui/core';
+import ReactDOM from 'react-dom';
+import { useSelector } from 'react-redux';
+import {
+ BinaryQueryObjectFilterClause,
+ FeatureFlag,
+ isFeatureEnabled,
+ QueryFormData,
+} from '@superset-ui/core';
+import { RootState } from 'src/dashboard/types';
+import { findPermission } from 'src/utils/findPermission';
import { Menu } from 'src/components/Menu';
import { AntdDropdown as Dropdown } from 'src/components';
-import ReactDOM from 'react-dom';
+import { DrillDetailMenuItems } from './DrillDetail';
const MENU_ITEM_HEIGHT = 32;
const MENU_VERTICAL_SPACING = 32;
export interface ChartContextMenuProps {
- id: string;
- onSelection: (filters: QueryObjectFilterClause[]) => void;
+ id: number;
+ formData: QueryFormData;
+ onSelection: () => void;
onClose: () => void;
}
export interface Ref {
open: (
- filters: QueryObjectFilterClause[],
clientX: number,
clientY: number,
+ filters?: BinaryQueryObjectFilterClause[],
) => void;
}
-const Filter = styled.span`
- ${({ theme }) => `
- font-weight: ${theme.typography.weights.bold};
- color: ${theme.colors.primary.base};
- `}
-`;
-
const ChartContextMenu = (
- { id, onSelection, onClose }: ChartContextMenuProps,
+ { id, formData, onSelection, onClose }: ChartContextMenuProps,
ref: RefObject<Ref>,
) => {
- const [state, setState] = useState<{
- filters: QueryObjectFilterClause[];
+ const canExplore = useSelector((state: RootState) =>
+ findPermission('can_explore', 'Superset', state.user?.roles),
+ );
+
+ const [{ filters, clientX, clientY }, setState] = useState<{
clientX: number;
clientY: number;
- }>({ filters: [], clientX: 0, clientY: 0 });
+ filters?: BinaryQueryObjectFilterClause[];
+ }>({ clientX: 0, clientY: 0 });
- const menu = (
- <Menu>
- {state.filters.map((filter, i) => (
- <Menu.Item key={i} onClick={() => onSelection([filter])}>
- {`${t('Drill to detail by')} `}
- <Filter>{filter.formattedVal}</Filter>
- </Menu.Item>
- ))}
- {state.filters.length === 0 && (
- <Menu.Item key="none" onClick={() => onSelection([])}>
- {t('Drill to detail')}
- </Menu.Item>
- )}
- {state.filters.length > 1 && (
- <Menu.Item key="all" onClick={() => onSelection(state.filters)}>
- {`${t('Drill to detail by')} `}
- <Filter>{t('all')}</Filter>
- </Menu.Item>
- )}
- </Menu>
- );
+ const menuItems = [];
+ const showDrillToDetail =
+ isFeatureEnabled(FeatureFlag.DRILL_TO_DETAIL) && canExplore;
+
+ if (showDrillToDetail) {
Review Comment:
reduce redundant constant.
```suggestion
if (isFeatureEnabled(FeatureFlag.DRILL_TO_DETAIL) && canExplore) {
```
##########
superset-frontend/src/components/Chart/ChartContextMenu.tsx:
##########
@@ -23,90 +23,102 @@ import React, {
useImperativeHandle,
useState,
} from 'react';
-import { QueryObjectFilterClause, t, styled } from '@superset-ui/core';
+import ReactDOM from 'react-dom';
+import { useSelector } from 'react-redux';
+import {
+ BinaryQueryObjectFilterClause,
+ FeatureFlag,
+ isFeatureEnabled,
+ QueryFormData,
+} from '@superset-ui/core';
+import { RootState } from 'src/dashboard/types';
+import { findPermission } from 'src/utils/findPermission';
import { Menu } from 'src/components/Menu';
import { AntdDropdown as Dropdown } from 'src/components';
-import ReactDOM from 'react-dom';
+import { DrillDetailMenuItems } from './DrillDetail';
const MENU_ITEM_HEIGHT = 32;
const MENU_VERTICAL_SPACING = 32;
export interface ChartContextMenuProps {
- id: string;
- onSelection: (filters: QueryObjectFilterClause[]) => void;
+ id: number;
+ formData: QueryFormData;
+ onSelection: () => void;
onClose: () => void;
}
export interface Ref {
open: (
- filters: QueryObjectFilterClause[],
clientX: number,
clientY: number,
+ filters?: BinaryQueryObjectFilterClause[],
) => void;
}
-const Filter = styled.span`
- ${({ theme }) => `
- font-weight: ${theme.typography.weights.bold};
- color: ${theme.colors.primary.base};
- `}
-`;
-
const ChartContextMenu = (
- { id, onSelection, onClose }: ChartContextMenuProps,
+ { id, formData, onSelection, onClose }: ChartContextMenuProps,
ref: RefObject<Ref>,
) => {
- const [state, setState] = useState<{
- filters: QueryObjectFilterClause[];
+ const canExplore = useSelector((state: RootState) =>
+ findPermission('can_explore', 'Superset', state.user?.roles),
+ );
+
+ const [{ filters, clientX, clientY }, setState] = useState<{
clientX: number;
clientY: number;
- }>({ filters: [], clientX: 0, clientY: 0 });
+ filters?: BinaryQueryObjectFilterClause[];
+ }>({ clientX: 0, clientY: 0 });
- const menu = (
- <Menu>
- {state.filters.map((filter, i) => (
- <Menu.Item key={i} onClick={() => onSelection([filter])}>
- {`${t('Drill to detail by')} `}
- <Filter>{filter.formattedVal}</Filter>
- </Menu.Item>
- ))}
- {state.filters.length === 0 && (
- <Menu.Item key="none" onClick={() => onSelection([])}>
- {t('Drill to detail')}
- </Menu.Item>
- )}
- {state.filters.length > 1 && (
- <Menu.Item key="all" onClick={() => onSelection(state.filters)}>
- {`${t('Drill to detail by')} `}
- <Filter>{t('all')}</Filter>
- </Menu.Item>
- )}
- </Menu>
- );
+ const menuItems = [];
+ const showDrillToDetail =
+ isFeatureEnabled(FeatureFlag.DRILL_TO_DETAIL) && canExplore;
+
+ if (showDrillToDetail) {
+ menuItems.push(
+ <DrillDetailMenuItems
+ chartId={id}
+ formData={formData}
+ isContextMenu
+ filters={filters}
+ onSelection={onSelection}
+ />,
+ );
+ }
const open = useCallback(
- (filters: QueryObjectFilterClause[], clientX: number, clientY: number) => {
+ (
+ clientX: number,
+ clientY: number,
+ filters?: BinaryQueryObjectFilterClause[],
+ ) => {
// Viewport height
const vh = Math.max(
document.documentElement.clientHeight || 0,
window.innerHeight || 0,
);
- // +1 for automatically added options such as 'All' and 'Drill to detail'
- const itemsCount = filters.length + 1;
+ const itemsCount =
+ [
+ showDrillToDetail ? 2 : 0, // Drill to detail always has 2 top-level menu items
+ ].reduce((a, b) => a + b, 0) || 1; // "No actions" appears if no actions in menu
+
const menuHeight = MENU_ITEM_HEIGHT * itemsCount + MENU_VERTICAL_SPACING;
// Always show the context menu inside the viewport
const adjustedY = vh - clientY < menuHeight ? vh - menuHeight : clientY;
- setState({ filters, clientX, clientY: adjustedY });
+ setState({
+ clientX,
+ clientY: adjustedY,
+ filters,
+ });
// Since Ant Design's Dropdown does not offer an imperative API
// and we can't attach event triggers to charts SVG elements, we
// use a hidden span that gets clicked on when receiving click events
// from the charts.
document.getElementById(`hidden-span-${id}`)?.click();
},
- [id],
+ [id, showDrillToDetail],
Review Comment:
The `showDrillToDetail` is from global config(FF) and permissions getter(`can_explore` in state) so it never change after finished frontend loading.
```suggestion
[id],
```
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailMenuItems.tsx:
##########
@@ -0,0 +1,230 @@
+/**
+ * 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 React, { ReactNode, useCallback, useMemo, useState } from 'react';
+import {
+ Behavior,
+ BinaryQueryObjectFilterClause,
+ css,
+ getChartMetadataRegistry,
+ QueryFormData,
+ styled,
+ SupersetTheme,
+ t,
+} from '@superset-ui/core';
+import { Menu } from 'src/components/Menu';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import DrillDetailModal from './DrillDetailModal';
+
+const DisabledMenuItemTooltip = ({ title }: { title: ReactNode }) => (
+ <Tooltip title={title} placement="top">
+ <Icons.InfoCircleOutlined
+ data-test="tooltip-trigger"
+ css={(theme: SupersetTheme) => css`
+ color: ${theme.colors.text.label};
+ margin-left: ${theme.gridUnit * 2}px;
+ &.anticon {
+ font-size: unset;
+ .anticon {
+ line-height: unset;
+ vertical-align: unset;
+ }
+ }
+ `}
+ />
+ </Tooltip>
+);
+
+const DisabledMenuItem = ({ children, ...props }: { children: ReactNode }) => (
+ <Menu.Item disabled {...props}>
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {children}
+ </div>
+ </Menu.Item>
+);
+
+const Filter = styled.span`
+ ${({ theme }) => `
+ font-weight: ${theme.typography.weights.bold};
+ color: ${theme.colors.primary.base};
+ `}
+`;
+
+export type DrillDetailMenuItemsProps = {
+ chartId: number;
+ formData: QueryFormData;
+ filters?: BinaryQueryObjectFilterClause[];
+ isContextMenu?: boolean;
+ onSelection?: () => void;
+ onClick?: (event: MouseEvent) => void;
+};
+
+const DrillDetailMenuItems = ({
+ chartId,
+ formData,
+ filters = [],
+ isContextMenu = false,
+ onSelection = () => null,
+ onClick = () => null,
+ ...props
+}: DrillDetailMenuItemsProps) => {
+ const [modalFilters, setFilters] = useState<BinaryQueryObjectFilterClause[]>(
+ [],
+ );
+
+ const [showModal, setShowModal] = useState(false);
+ const openModal = useCallback(
+ (filters, event) => {
+ onClick(event);
+ onSelection();
+ setFilters(filters);
+ setShowModal(true);
+ },
+ [onClick, onSelection],
+ );
+
+ const closeModal = useCallback(() => {
+ setShowModal(false);
+ }, []);
+
+ // Check for Behavior.DRILL_TO_DETAIL to tell if plugin handles the `contextmenu`
+ // event for dimensions. If it doesn't, tell the user that drill to detail by
+ // dimension is not supported. If it does, and the `contextmenu` handler didn't
+ // pass any filters, tell the user that they didn't select a dimension.
+ const handlesDimensionContextMenu = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.behaviors.find(behavior => behavior === Behavior.DRILL_TO_DETAIL),
+ [formData.viz_type],
+ );
+
+ // Check chart plugin metadata to see if chart's current configuration lacks
+ // aggregations, in which case Drill to Detail should be disabled.
+ const noAggregations = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.noAggregations?.(formData) ?? false,
+ [formData],
+ );
+
+ const drillToDetail = noAggregations ? (
Review Comment:
it's a `mean` component, should we rename it to `menu` or `drillToDetailMenu`?
##########
superset-frontend/packages/superset-ui-core/src/chart/models/ChartMetadata.ts:
##########
@@ -49,6 +50,7 @@ export interface ChartMetadataConfig {
label?: ChartLabel | null;
labelExplanation?: string | null;
queryObjectCount?: number;
+ noAggregations?: (formData: QueryFormData) => boolean;
Review Comment:
`noAggregations` refer to that the current query is not an aggregative query, so the other way to say that the `metrics` is empty in the `queryObject`. In order to resolve this problem, we just check the `form_data` or `queryObject` instead of defining new props in ChartMetada.
The above snippet shows how to detect query types. we could utilize it.
```typescript
import { extractQueryFields } from 'superset-ui/core'
const { metrics } = extractQueryFields(formData)
if (!(Array.isArray(metrics) && metrics.length > 0)) {
// logic for the noAggregations query.
}
```
##########
superset-frontend/src/components/Chart/ChartContextMenu.tsx:
##########
@@ -119,7 +131,15 @@ const ChartContextMenu = (
return ReactDOM.createPortal(
<Dropdown
- overlay={menu}
+ overlay={
+ <Menu>
+ {menuItems.length ? (
Review Comment:
explicitly treat array size condition.
```suggestion
{menuItems.length > 0 ? (
```
##########
superset-frontend/plugins/plugin-chart-table/src/types.ts:
##########
@@ -113,9 +113,9 @@ export interface TableChartTransformedProps<D extends DataRecord = DataRecord> {
columnColorFormatters?: ColorFormatters;
allowRearrangeColumns?: boolean;
onContextMenu?: (
- filters: QueryObjectFilterClause[],
clientX: number,
clientY: number,
+ filters?: QueryObjectFilterClause[],
Review Comment:
should align this type and every plugin.
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailMenuItems.tsx:
##########
@@ -0,0 +1,230 @@
+/**
+ * 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 React, { ReactNode, useCallback, useMemo, useState } from 'react';
+import {
+ Behavior,
+ BinaryQueryObjectFilterClause,
+ css,
+ getChartMetadataRegistry,
+ QueryFormData,
+ styled,
+ SupersetTheme,
+ t,
+} from '@superset-ui/core';
+import { Menu } from 'src/components/Menu';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import DrillDetailModal from './DrillDetailModal';
+
+const DisabledMenuItemTooltip = ({ title }: { title: ReactNode }) => (
+ <Tooltip title={title} placement="top">
+ <Icons.InfoCircleOutlined
+ data-test="tooltip-trigger"
+ css={(theme: SupersetTheme) => css`
+ color: ${theme.colors.text.label};
+ margin-left: ${theme.gridUnit * 2}px;
+ &.anticon {
+ font-size: unset;
+ .anticon {
+ line-height: unset;
+ vertical-align: unset;
+ }
+ }
+ `}
+ />
+ </Tooltip>
+);
+
+const DisabledMenuItem = ({ children, ...props }: { children: ReactNode }) => (
+ <Menu.Item disabled {...props}>
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {children}
+ </div>
+ </Menu.Item>
+);
+
+const Filter = styled.span`
+ ${({ theme }) => `
+ font-weight: ${theme.typography.weights.bold};
+ color: ${theme.colors.primary.base};
+ `}
+`;
+
+export type DrillDetailMenuItemsProps = {
+ chartId: number;
+ formData: QueryFormData;
+ filters?: BinaryQueryObjectFilterClause[];
+ isContextMenu?: boolean;
+ onSelection?: () => void;
+ onClick?: (event: MouseEvent) => void;
+};
+
+const DrillDetailMenuItems = ({
+ chartId,
+ formData,
+ filters = [],
+ isContextMenu = false,
+ onSelection = () => null,
+ onClick = () => null,
+ ...props
+}: DrillDetailMenuItemsProps) => {
+ const [modalFilters, setFilters] = useState<BinaryQueryObjectFilterClause[]>(
+ [],
+ );
+
+ const [showModal, setShowModal] = useState(false);
+ const openModal = useCallback(
+ (filters, event) => {
+ onClick(event);
+ onSelection();
+ setFilters(filters);
+ setShowModal(true);
+ },
+ [onClick, onSelection],
+ );
+
+ const closeModal = useCallback(() => {
+ setShowModal(false);
+ }, []);
+
+ // Check for Behavior.DRILL_TO_DETAIL to tell if plugin handles the `contextmenu`
+ // event for dimensions. If it doesn't, tell the user that drill to detail by
+ // dimension is not supported. If it does, and the `contextmenu` handler didn't
+ // pass any filters, tell the user that they didn't select a dimension.
+ const handlesDimensionContextMenu = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.behaviors.find(behavior => behavior === Behavior.DRILL_TO_DETAIL),
+ [formData.viz_type],
+ );
+
+ // Check chart plugin metadata to see if chart's current configuration lacks
+ // aggregations, in which case Drill to Detail should be disabled.
+ const noAggregations = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.noAggregations?.(formData) ?? false,
+ [formData],
+ );
+
+ const drillToDetail = noAggregations ? (
+ <DisabledMenuItem {...props} key="drill-detail-no-aggregations">
+ {t('Drill to detail')}
+ <DisabledMenuItemTooltip
+ title={t(
+ 'Drill to detail is disabled because this chart does not group data by dimension value.',
+ )}
+ />
+ </DisabledMenuItem>
+ ) : (
+ <Menu.Item
+ {...props}
+ key="drill-detail-no-filters"
+ onClick={openModal.bind(null, [])}
+ >
+ {t('Drill to detail')}
+ </Menu.Item>
+ );
+
+ let drillToDetailBy;
+ if (noAggregations) {
+ drillToDetailBy = (
+ <DisabledMenuItem {...props} key="drill-detail-by-no-aggregations">
+ {t('Drill to detail by')}
+ </DisabledMenuItem>
+ );
+ } else {
+ drillToDetailBy = (
+ <DisabledMenuItem {...props} key="drill-detail-by-chart-not-supported">
+ {t('Drill to detail by')}
+ <DisabledMenuItemTooltip
+ title={t(
+ 'Drill to detail by value is not yet supported for this chart type.',
+ )}
+ />
+ </DisabledMenuItem>
+ );
+
+ if (handlesDimensionContextMenu) {
+ if (filters?.length) {
Review Comment:
This is the third level if clause, should optimize readability.
##########
superset-frontend/src/components/Chart/ChartContextMenu.tsx:
##########
@@ -23,90 +23,102 @@ import React, {
useImperativeHandle,
useState,
} from 'react';
-import { QueryObjectFilterClause, t, styled } from '@superset-ui/core';
+import ReactDOM from 'react-dom';
+import { useSelector } from 'react-redux';
+import {
+ BinaryQueryObjectFilterClause,
+ FeatureFlag,
+ isFeatureEnabled,
+ QueryFormData,
+} from '@superset-ui/core';
+import { RootState } from 'src/dashboard/types';
+import { findPermission } from 'src/utils/findPermission';
import { Menu } from 'src/components/Menu';
import { AntdDropdown as Dropdown } from 'src/components';
-import ReactDOM from 'react-dom';
+import { DrillDetailMenuItems } from './DrillDetail';
const MENU_ITEM_HEIGHT = 32;
const MENU_VERTICAL_SPACING = 32;
export interface ChartContextMenuProps {
- id: string;
- onSelection: (filters: QueryObjectFilterClause[]) => void;
+ id: number;
+ formData: QueryFormData;
+ onSelection: () => void;
onClose: () => void;
}
export interface Ref {
open: (
- filters: QueryObjectFilterClause[],
clientX: number,
clientY: number,
+ filters?: BinaryQueryObjectFilterClause[],
) => void;
}
-const Filter = styled.span`
- ${({ theme }) => `
- font-weight: ${theme.typography.weights.bold};
- color: ${theme.colors.primary.base};
- `}
-`;
-
const ChartContextMenu = (
- { id, onSelection, onClose }: ChartContextMenuProps,
+ { id, formData, onSelection, onClose }: ChartContextMenuProps,
ref: RefObject<Ref>,
) => {
- const [state, setState] = useState<{
- filters: QueryObjectFilterClause[];
+ const canExplore = useSelector((state: RootState) =>
+ findPermission('can_explore', 'Superset', state.user?.roles),
+ );
+
+ const [{ filters, clientX, clientY }, setState] = useState<{
clientX: number;
clientY: number;
- }>({ filters: [], clientX: 0, clientY: 0 });
+ filters?: BinaryQueryObjectFilterClause[];
+ }>({ clientX: 0, clientY: 0 });
- const menu = (
- <Menu>
- {state.filters.map((filter, i) => (
- <Menu.Item key={i} onClick={() => onSelection([filter])}>
- {`${t('Drill to detail by')} `}
- <Filter>{filter.formattedVal}</Filter>
- </Menu.Item>
- ))}
- {state.filters.length === 0 && (
- <Menu.Item key="none" onClick={() => onSelection([])}>
- {t('Drill to detail')}
- </Menu.Item>
- )}
- {state.filters.length > 1 && (
- <Menu.Item key="all" onClick={() => onSelection(state.filters)}>
- {`${t('Drill to detail by')} `}
- <Filter>{t('all')}</Filter>
- </Menu.Item>
- )}
- </Menu>
- );
+ const menuItems = [];
+ const showDrillToDetail =
+ isFeatureEnabled(FeatureFlag.DRILL_TO_DETAIL) && canExplore;
+
+ if (showDrillToDetail) {
+ menuItems.push(
+ <DrillDetailMenuItems
+ chartId={id}
+ formData={formData}
+ isContextMenu
+ filters={filters}
+ onSelection={onSelection}
+ />,
+ );
+ }
const open = useCallback(
- (filters: QueryObjectFilterClause[], clientX: number, clientY: number) => {
+ (
+ clientX: number,
+ clientY: number,
+ filters?: BinaryQueryObjectFilterClause[],
+ ) => {
// Viewport height
const vh = Math.max(
document.documentElement.clientHeight || 0,
window.innerHeight || 0,
);
- // +1 for automatically added options such as 'All' and 'Drill to detail'
- const itemsCount = filters.length + 1;
+ const itemsCount =
+ [
+ showDrillToDetail ? 2 : 0, // Drill to detail always has 2 top-level menu items
+ ].reduce((a, b) => a + b, 0) || 1; // "No actions" appears if no actions in menu
+
const menuHeight = MENU_ITEM_HEIGHT * itemsCount + MENU_VERTICAL_SPACING;
Review Comment:
```suggestion
const menuHeight = MENU_ITEM_HEIGHT * menuItems.length > 0 ? 2 : 1 + MENU_VERTICAL_SPACING;
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -26,7 +26,7 @@ import {
computeMaxFontSize,
BRAND_COLOR,
styled,
- QueryObjectFilterClause,
+ BinaryQueryObjectFilterClause,
Review Comment:
the `BinaryQueryObjectFilterClause` is a kind of `QueryObjectFilterClause`, do we need narrow it?
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailMenuItems.tsx:
##########
@@ -0,0 +1,230 @@
+/**
+ * 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 React, { ReactNode, useCallback, useMemo, useState } from 'react';
+import {
+ Behavior,
+ BinaryQueryObjectFilterClause,
+ css,
+ getChartMetadataRegistry,
+ QueryFormData,
+ styled,
+ SupersetTheme,
+ t,
+} from '@superset-ui/core';
+import { Menu } from 'src/components/Menu';
+import Icons from 'src/components/Icons';
+import { Tooltip } from 'src/components/Tooltip';
+import DrillDetailModal from './DrillDetailModal';
+
+const DisabledMenuItemTooltip = ({ title }: { title: ReactNode }) => (
+ <Tooltip title={title} placement="top">
+ <Icons.InfoCircleOutlined
+ data-test="tooltip-trigger"
+ css={(theme: SupersetTheme) => css`
+ color: ${theme.colors.text.label};
+ margin-left: ${theme.gridUnit * 2}px;
+ &.anticon {
+ font-size: unset;
+ .anticon {
+ line-height: unset;
+ vertical-align: unset;
+ }
+ }
+ `}
+ />
+ </Tooltip>
+);
+
+const DisabledMenuItem = ({ children, ...props }: { children: ReactNode }) => (
+ <Menu.Item disabled {...props}>
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {children}
+ </div>
+ </Menu.Item>
+);
+
+const Filter = styled.span`
+ ${({ theme }) => `
+ font-weight: ${theme.typography.weights.bold};
+ color: ${theme.colors.primary.base};
+ `}
+`;
+
+export type DrillDetailMenuItemsProps = {
+ chartId: number;
+ formData: QueryFormData;
+ filters?: BinaryQueryObjectFilterClause[];
+ isContextMenu?: boolean;
+ onSelection?: () => void;
+ onClick?: (event: MouseEvent) => void;
+};
+
+const DrillDetailMenuItems = ({
+ chartId,
+ formData,
+ filters = [],
+ isContextMenu = false,
+ onSelection = () => null,
+ onClick = () => null,
+ ...props
+}: DrillDetailMenuItemsProps) => {
+ const [modalFilters, setFilters] = useState<BinaryQueryObjectFilterClause[]>(
+ [],
+ );
+
+ const [showModal, setShowModal] = useState(false);
+ const openModal = useCallback(
+ (filters, event) => {
+ onClick(event);
+ onSelection();
+ setFilters(filters);
+ setShowModal(true);
+ },
+ [onClick, onSelection],
+ );
+
+ const closeModal = useCallback(() => {
+ setShowModal(false);
+ }, []);
+
+ // Check for Behavior.DRILL_TO_DETAIL to tell if plugin handles the `contextmenu`
+ // event for dimensions. If it doesn't, tell the user that drill to detail by
+ // dimension is not supported. If it does, and the `contextmenu` handler didn't
+ // pass any filters, tell the user that they didn't select a dimension.
+ const handlesDimensionContextMenu = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.behaviors.find(behavior => behavior === Behavior.DRILL_TO_DETAIL),
+ [formData.viz_type],
+ );
+
+ // Check chart plugin metadata to see if chart's current configuration lacks
+ // aggregations, in which case Drill to Detail should be disabled.
+ const noAggregations = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.noAggregations?.(formData) ?? false,
+ [formData],
+ );
+
+ const drillToDetail = noAggregations ? (
+ <DisabledMenuItem {...props} key="drill-detail-no-aggregations">
+ {t('Drill to detail')}
+ <DisabledMenuItemTooltip
+ title={t(
+ 'Drill to detail is disabled because this chart does not group data by dimension value.',
+ )}
+ />
+ </DisabledMenuItem>
+ ) : (
+ <Menu.Item
+ {...props}
+ key="drill-detail-no-filters"
+ onClick={openModal.bind(null, [])}
+ >
+ {t('Drill to detail')}
+ </Menu.Item>
+ );
+
+ let drillToDetailBy;
Review Comment:
this one is the `menuItem` component so should rename to `menuItem` or `drillDetailMenuItem`.
--
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