michael-s-molina commented on code in PR #21351:
URL: https://github.com/apache/superset/pull/21351#discussion_r989370475
##########
superset-frontend/packages/superset-ui-core/src/chart/models/ChartMetadata.ts:
##########
@@ -90,6 +92,11 @@ export default class ChartMetadata {
queryObjectCount: number;
+ // Optional function allowing a chart to indicate if its current
configuration
+ // lacks data aggregations (e.g. to determine if Drill to Detail should be
+ // enabled)
Review Comment:
```suggestion
// Optional function allowing a chart to indicate if its current
configuration
// lacks data aggregations (e.g. to determine if Drill to Detail should be
// enabled)
```
##########
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[],
+ filters: BinaryQueryObjectFilterClause[] | null,
clientX: number,
clientY: number,
) => 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<{
+ filters: BinaryQueryObjectFilterClause[];
clientX: number;
clientY: number;
}>({ filters: [], 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) => {
+ (
+ filters: BinaryQueryObjectFilterClause[] | null,
Review Comment:
We could make `filters` optional and move it to the end:
```
(clientX: number, clientY: number, filters?:
BinaryQueryObjectFilterClause[]) => {}
```
##########
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={(theme: SupersetTheme) => css`
+ white-space: normal;
+ max-width: ${theme.gridUnit * 40}px;
+ `}
+ >
+ {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,
+ 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
Review Comment:
```suggestion
// Check chart plugin metadata to see if chart's current configuration
lacks
// aggregations, in which case Drill to Detail should be disabled
```
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -285,47 +296,55 @@ class ChartRenderer extends React.Component {
);
}
+ // Check for Behavior.DRILL_TO_DETAIL to tell if chart can receive Drill
to
+ // Detail props or if it'll cause side-effects (e.g. excessive
re-renders).
Review Comment:
Out of curiosity, why passing these props would cause excessive re-renders?
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailMenuItems.test.tsx:
##########
@@ -0,0 +1,343 @@
+/**
+ * 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 from 'react';
+import userEvent from '@testing-library/user-event';
+import { render, screen, within } from 'spec/helpers/testing-library';
+import { getMockStoreWithNativeFilters } from 'spec/fixtures/mockStore';
+import chartQueries, { sliceId } from 'spec/fixtures/mockChartQueries';
+import { QueryObjectFilterClause } from '@superset-ui/core';
+import { Menu } from 'src/components/Menu';
+import DrillDetailMenuItems, {
+ DrillDetailMenuItemsProps,
+} from './DrillDetailMenuItems';
+
+/* eslint jest/expect-expect: ["warn", { "assertFunctionNames": ["expect*"] }]
*/
Review Comment:
One way to avoid disabling the rule is to make the helper functions do all
the heavy work for finding an item and returning it. Then, inside the test you
do the `expect(helperToFindAnElement()).toBeIntheDocument());` We have similar
patterns in the codebase.
##########
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={(theme: SupersetTheme) => css`
+ white-space: normal;
+ max-width: ${theme.gridUnit * 40}px;
+ `}
+ >
+ {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,
+ 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.
Review Comment:
```suggestion
// 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.
```
##########
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={(theme: SupersetTheme) => css`
+ white-space: normal;
+ max-width: ${theme.gridUnit * 40}px;
Review Comment:
I'm not sure if `max-width` is related to `gridUnit` in this case. It seems
it's a fixed `160px`;
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -203,14 +205,23 @@ class ChartRenderer extends React.Component {
this.setState({ inContextMenu: true });
}
- handleContextMenuSelected(filters) {
- this.setState({ inContextMenu: false, drillDetailFilters: filters });
+ handleContextMenuSelected() {
+ this.setState({ inContextMenu: false });
}
handleContextMenuClosed() {
this.setState({ inContextMenu: false });
}
+ // When viz plugins don't handle `contextmenu` event, fallback handler
+ // calls `handleOnContextMenu` with `filters: null`.
+ onContextMenuFallback(event) {
+ if (!this.state.inContextMenu) {
+ event.preventDefault();
+ this.handleOnContextMenu(null, event.clientX, event.clientY);
Review Comment:
If we make `filters` optional then we could just ignore it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
