Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dashboard): menu improvements, fallback support for Drill to Detail #21351

Merged
merged 38 commits into from
Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
ddebcea
Refactor D2D menu/modal.
codyml Sep 17, 2022
2899160
Implement/test fallback menu.
codyml Sep 23, 2022
17622bb
Implement/test menu for supported chart w/o filters.
codyml Sep 23, 2022
0f3d59a
Stub out todo tests.
codyml Sep 28, 2022
c1da35e
Refactor components, tests.
codyml Sep 28, 2022
4bc91eb
WIP
codyml Sep 29, 2022
5894d03
WIP
codyml Sep 29, 2022
2f3b4f2
WIP
codyml Sep 29, 2022
94e3b43
Implement/test rest of menu items.
codyml Sep 29, 2022
ba0eb71
Fixes
codyml Sep 29, 2022
143a273
Typing, other cleanup.
codyml Sep 29, 2022
f0aaa8b
Disable D2D menu if user doesn't have explore permissions.
codyml Sep 29, 2022
ac720d1
Update menu placement.
codyml Sep 29, 2022
019a046
Typing, param cleanup.
codyml Sep 30, 2022
0479c23
Cleanup
codyml Sep 30, 2022
5d2a86c
Update behavior name.
codyml Sep 30, 2022
cc49b90
Fix failing test.
codyml Oct 3, 2022
995fcb8
Fix Cypress tests.
codyml Oct 3, 2022
c1a8321
Cleanup.
codyml Oct 4, 2022
d915f5f
Fix broken Table and Pivot Table charts.
codyml Oct 4, 2022
9c619e7
Use QueryMode enum.
codyml Oct 4, 2022
cce3d03
Move submenu messages to tooltips, retrigger CI, retrigger CI.
codyml Oct 4, 2022
0f0c0b0
Make modal mask block contextmenu events.
codyml Oct 5, 2022
d8e3bb1
Add missing D2D behavior.
codyml Oct 5, 2022
c85eb38
Clean up import.
codyml Oct 5, 2022
422cfa1
Use enum for chart source.
codyml Oct 5, 2022
adf3e2f
Update Cypress tests.
codyml Oct 6, 2022
64aa996
Better fix for disabling right-click with modal open.
codyml Oct 6, 2022
d9b4303
Remove accidental duplicate tests.
codyml Oct 6, 2022
14c4496
Remove extra space from comments.
codyml Oct 6, 2022
7851d68
Change onContextMenu function signature.
codyml Oct 7, 2022
0437083
Make menu max width not dependent on grid unit.
codyml Oct 7, 2022
4b97388
Replace noAggregations metadata function with metric check.
codyml Oct 7, 2022
9cfcc39
Use BinaryQueryObjectFilterClause consistently.
codyml Oct 7, 2022
4c57264
Rename variables.
codyml Oct 7, 2022
8bc9476
Restructure if/else blocks for readability.
codyml Oct 7, 2022
bb43302
Make enum PascalCase.
codyml Oct 7, 2022
85d080f
Fix no aggregations message appearing on unsupported charts.
codyml Oct 7, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,29 @@ function openModalFromChartContext(targetMenuItem: string) {
interceptSamples();

cy.wait(500);
cy.get('.ant-dropdown')
.not('.ant-dropdown-hidden')
.first()
.find("[role='menu'] [role='menuitem']")
.contains(targetMenuItem)
.first()
.click();
if (targetMenuItem.startsWith('Drill to detail by')) {
cy.get('.ant-dropdown')
.not('.ant-dropdown-hidden')
.first()
.find("[role='menu'] [role='menuitem'] [title='Drill to detail by']")
.trigger('mouseover');
cy.wait(500);
cy.get('[data-test="drill-to-detail-by-submenu"]')
.not('.ant-dropdown-menu-hidden [data-test="drill-to-detail-by-submenu"]')
.find('[role="menuitem"]')
.contains(new RegExp(`^${targetMenuItem}$`))
.first()
.click();
} else {
cy.get('.ant-dropdown')
.not('.ant-dropdown-hidden')
.first()
.find("[role='menu'] [role='menuitem']")
.contains(new RegExp(`^${targetMenuItem}$`))
.first()
.click();
}

cy.wait('@samples');
}

Expand Down Expand Up @@ -404,6 +420,18 @@ describe('Drill to detail modal', () => {
});
});
});

describe('Bar Chart', () => {
it('opens the modal for unsupported chart without filters', () => {
interceptSamples();

cy.get("[data-test-viz-type='dist_bar'] svg").then($canvas => {
cy.wrap($canvas).scrollIntoView().rightclick(70, 150);
openModalFromChartContext('Drill to detail');
cy.getBySel('filter-val').should('not.exist');
});
});
});
});

describe('Tier 2 charts', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ export type HandlerFunction = (...args: unknown[]) => void;
export enum Behavior {
INTERACTIVE_CHART = 'INTERACTIVE_CHART',
NATIVE_FILTER = 'NATIVE_FILTER',

/**
* Include `DRILL_TO_DETAIL` behavior if plugin handles `contextmenu` event
* when dimensions are right-clicked on.
*/
DRILL_TO_DETAIL = 'DRILL_TO_DETAIL',
}

export enum AppSection {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export {
isXAxisSet,
hasGenericChartAxes,
} from './getXAxis';
export { default as extractQueryFields } from './extractQueryFields';

export * from './types/AnnotationLayer';
export * from './types/QueryFormData';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ function WorldMap(element, props) {
formattedVal: val,
},
];
onContextMenu(filters, pointerEvent.clientX, pointerEvent.clientY);
onContextMenu(pointerEvent.clientX, pointerEvent.clientY, filters);
} else {
logging.warn(
t(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { t, ChartMetadata, ChartPlugin } from '@superset-ui/core';
import { t, ChartMetadata, ChartPlugin, Behavior } from '@superset-ui/core';
import transformProps from './transformProps';
import thumbnail from './images/thumbnail.png';
import example1 from './images/WorldMap1.jpg';
Expand Down Expand Up @@ -45,6 +45,7 @@ const metadata = new ChartMetadata({
],
thumbnail,
useLegacyApi: true,
behaviors: [Behavior.DRILL_TO_DETAIL],
});

export default class WorldMapChartPlugin extends ChartPlugin {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { t, ChartMetadata, ChartPlugin } from '@superset-ui/core';
import { t, ChartMetadata, ChartPlugin, Behavior } from '@superset-ui/core';
import controlPanel from './controlPanel';
import transformProps from './transformProps';
import buildQuery from './buildQuery';
Expand Down Expand Up @@ -46,6 +46,7 @@ const metadata = new ChartMetadata({
t('Description'),
],
thumbnail,
behaviors: [Behavior.DRILL_TO_DETAIL],
});

export default class BigNumberTotalChartPlugin extends ChartPlugin<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
computeMaxFontSize,
BRAND_COLOR,
styled,
QueryObjectFilterClause,
BinaryQueryObjectFilterClause,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the BinaryQueryObjectFilterClause is a kind of QueryObjectFilterClause, do we need narrow it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes, because DrillDetailTableControls is currently only set up to correctly display binary filters (e.g. column = value), and if you change the type to QueryObjectFilterClause there you'll get typing errors that you can't resolve without adding new logic for displaying the other types. Set filters and unary filters are definitely possible to add display support for in the future but in its current state it only works with binary filters.

} from '@superset-ui/core';
import { EChartsCoreOption } from 'echarts';
import Echart from '../components/Echart';
Expand Down Expand Up @@ -65,9 +65,9 @@ type BigNumberVisProps = {
mainColor: string;
echartOptions: EChartsCoreOption;
onContextMenu?: (
filters: QueryObjectFilterClause[],
clientX: number,
clientY: number,
filters?: BinaryQueryObjectFilterClause[],
) => void;
xValueFormatter?: TimeFormatter;
formData?: BigNumberWithTrendlineFormData;
Expand Down Expand Up @@ -171,11 +171,7 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps> {
const onContextMenu = (e: MouseEvent<HTMLDivElement>) => {
if (this.props.onContextMenu) {
e.preventDefault();
this.props.onContextMenu(
[],
e.nativeEvent.clientX,
e.nativeEvent.clientY,
);
this.props.onContextMenu(e.nativeEvent.clientX, e.nativeEvent.clientY);
}
};

Expand Down Expand Up @@ -249,7 +245,7 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps> {
const { data } = eventParams;
if (data) {
const pointerEvent = eventParams.event.event;
const filters: QueryObjectFilterClause[] = [];
const filters: BinaryQueryObjectFilterClause[] = [];
filters.push({
col: this.props.formData?.granularitySqla,
grain: this.props.formData?.timeGrainSqla,
Expand All @@ -258,9 +254,9 @@ class BigNumberVis extends React.PureComponent<BigNumberVisProps> {
formattedVal: this.props.xValueFormatter?.(data[0]),
});
this.props.onContextMenu(
filters,
pointerEvent.clientX,
pointerEvent.clientY,
filters,
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { t, ChartMetadata, ChartPlugin } from '@superset-ui/core';
import { t, ChartMetadata, ChartPlugin, Behavior } from '@superset-ui/core';
import controlPanel from './controlPanel';
import transformProps from './transformProps';
import buildQuery from './buildQuery';
Expand Down Expand Up @@ -45,6 +45,7 @@ const metadata = new ChartMetadata({
t('Trend'),
],
thumbnail,
behaviors: [Behavior.DRILL_TO_DETAIL],
});

export default class BigNumberWithTrendlineChartPlugin extends ChartPlugin<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export default class EchartsBoxPlotChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('./EchartsBoxPlot'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('Distribution'),
credits: ['https://echarts.apache.org'],
description: t(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default class EchartsFunnelChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('./EchartsFunnel'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('KPI'),
credits: ['https://echarts.apache.org'],
description: t(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export default class EchartsGaugeChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('./EchartsGauge'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('KPI'),
credits: ['https://echarts.apache.org'],
description: t(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/
import React from 'react';
import { QueryObjectFilterClause } from '@superset-ui/core';
import { BinaryQueryObjectFilterClause } from '@superset-ui/core';
import { EventHandlers } from '../types';
import Echart from '../components/Echart';
import { GraphChartTransformedProps } from './types';
Expand Down Expand Up @@ -47,7 +47,7 @@ export default function EchartsGraph({
const sourceValue = data.find(item => item.id === e.data.source)?.name;
const targetValue = data.find(item => item.id === e.data.target)?.name;
if (sourceValue && targetValue) {
const filters: QueryObjectFilterClause[] = [
const filters: BinaryQueryObjectFilterClause[] = [
{
col: formData.source,
op: '==',
Expand All @@ -61,7 +61,7 @@ export default function EchartsGraph({
formattedVal: targetValue,
},
];
onContextMenu(filters, pointerEvent.clientX, pointerEvent.clientY);
onContextMenu(pointerEvent.clientX, pointerEvent.clientY, filters);
}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { t, ChartMetadata, ChartPlugin } from '@superset-ui/core';
import { t, ChartMetadata, ChartPlugin, Behavior } from '@superset-ui/core';
import controlPanel from './controlPanel';
import transformProps from './transformProps';
import thumbnail from './images/thumbnail.png';
Expand Down Expand Up @@ -46,6 +46,7 @@ export default class EchartsGraphChartPlugin extends ChartPlugin {
t('Transformable'),
],
thumbnail,
behaviors: [Behavior.DRILL_TO_DETAIL],
}),
transformProps,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import {
PlainObject,
QueryFormData,
QueryObjectFilterClause,
BinaryQueryObjectFilterClause,
} from '@superset-ui/core';
import { GraphNodeItemOption } from 'echarts/types/src/chart/graph/GraphSeries';
import { SeriesTooltipOption } from 'echarts/types/src/util/types';
Expand Down Expand Up @@ -88,8 +88,8 @@ export type tooltipFormatParams = {
export type GraphChartTransformedProps = EchartsProps & {
formData: PlainObject;
onContextMenu?: (
filters: QueryObjectFilterClause[],
clientX: number,
clientY: number,
filters?: BinaryQueryObjectFilterClause[],
) => void;
};
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
AxisType,
DataRecordValue,
DTTM_ALIAS,
QueryObjectFilterClause,
BinaryQueryObjectFilterClause,
} from '@superset-ui/core';
import { EchartsMixedTimeseriesChartTransformedProps } from './types';
import Echart from '../components/Echart';
Expand Down Expand Up @@ -128,7 +128,7 @@ export default function EchartsMixedTimeseries({
eventParams.seriesName
],
];
const filters: QueryObjectFilterClause[] = [];
const filters: BinaryQueryObjectFilterClause[] = [];
if (xAxis.type === AxisType.time) {
filters.push({
col:
Expand All @@ -154,7 +154,7 @@ export default function EchartsMixedTimeseries({
formattedVal: String(values[i]),
}),
);
onContextMenu(filters, pointerEvent.clientX, pointerEvent.clientY);
onContextMenu(pointerEvent.clientX, pointerEvent.clientY, filters);
}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export default class EchartsTimeseriesChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('./EchartsMixedTimeseries'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('Evolution'),
credits: ['https://echarts.apache.org'],
description: hasGenericChartAxes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export default class EchartsPieChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('./EchartsPie'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('Part of a Whole'),
credits: ['https://echarts.apache.org'],
description:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export default class EchartsRadarChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('./EchartsRadar'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('Ranking'),
credits: ['https://echarts.apache.org'],
description: t(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export default class EchartsAreaChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('../EchartsTimeseries'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('Evolution'),
credits: ['https://echarts.apache.org'],
description: hasGenericChartAxes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import React, { useCallback, useEffect, useRef, useState } from 'react';
import {
DTTM_ALIAS,
QueryObjectFilterClause,
BinaryQueryObjectFilterClause,
AxisType,
} from '@superset-ui/core';
import { ViewRootGroup } from 'echarts/types/src/util/types';
Expand Down Expand Up @@ -191,7 +191,7 @@ export default function EchartsTimeseries({
...(eventParams.name ? [eventParams.name] : []),
...labelMap[eventParams.seriesName],
];
const filters: QueryObjectFilterClause[] = [];
const filters: BinaryQueryObjectFilterClause[] = [];
if (xAxis.type === AxisType.time) {
filters.push({
col:
Expand All @@ -216,7 +216,7 @@ export default function EchartsTimeseries({
formattedVal: String(values[i]),
}),
);
onContextMenu(filters, pointerEvent.clientX, pointerEvent.clientY);
onContextMenu(pointerEvent.clientX, pointerEvent.clientY, filters);
}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export default class EchartsTimeseriesBarChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('../../EchartsTimeseries'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('Evolution'),
credits: ['https://echarts.apache.org'],
description: hasGenericChartAxes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export default class EchartsTimeseriesLineChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('../../EchartsTimeseries'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('Evolution'),
credits: ['https://echarts.apache.org'],
description: hasGenericChartAxes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export default class EchartsTimeseriesScatterChartPlugin extends ChartPlugin<
controlPanel,
loadChart: () => import('../../EchartsTimeseries'),
metadata: new ChartMetadata({
behaviors: [Behavior.INTERACTIVE_CHART],
behaviors: [Behavior.INTERACTIVE_CHART, Behavior.DRILL_TO_DETAIL],
category: t('Evolution'),
credits: ['https://echarts.apache.org'],
description: hasGenericChartAxes
Expand Down
Loading