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.

Sign up for GitHub

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

Jump to bottom

fix: Total calculation in stacked Timeseries charts #24477

Merged
merged 2 commits into from
Jun 23, 2023
Merged

fix: Total calculation in stacked Timeseries charts #24477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07cca12
fix: Total calculation in stacked Timeseries charts
michael-s-molina Jun 22, 2023
24aa488
Fixes lint errors
michael-s-molina Jun 23, 2023
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
18 changes: 17 additions & 1 deletion superset-frontend/packages/superset-ui-core/src/chart/models/ChartProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ import {
FilterState,
JsonObject,
} from '../..';
import { HandlerFunction, PlainObject, SetDataMaskHook } from '../types/Base';
import {
HandlerFunction,
LegendState,
PlainObject,
SetDataMaskHook,
} from '../types/Base';
import { QueryData, DataRecordFilters } from '..';
import { SupersetTheme } from '../../style';

Expand All @@ -54,6 +59,8 @@ type Hooks = {
onContextMenu?: HandlerFunction;
/** handle errors */
onError?: HandlerFunction;
/** handle legend state changes */
onLegendStateChanged?: HandlerFunction;
/** use the vis as control to update state */
setControlValue?: HandlerFunction;
/** handle external filters */
Expand Down Expand Up @@ -88,6 +95,8 @@ export interface ChartPropsConfig {
ownState?: JsonObject;
/** Filter state that saved in dashboard */
filterState?: FilterState;
/** Legend state */
legendState?: LegendState;
Comment on lines +98 to +99
Copy link
Member

Choose a reason for hiding this comment

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

I want to raise the concern of bloating the number of different states we have here: Would it make sense to introduce something more generic, like chartState that could be leveraged for transporting other info, too? I assume this isn't the last time we need to ship back info from the plugin to transformProps, and having to add a new property can become tedious at some point. Thoughts? Alternatively, let's not do this now, but do a full cleanup when we get around to building the new React-based plugin arch (v2).

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 totally agree with your concern. I have many other concerns regarding the current plugin architecture but I think the best approach would be to stop amending the flawed design and think about a cleaner v2. One thing I think is really hard with the current structure is to properly test the plugin behaviors.

/** Set of actual behaviors that this instance of chart should use */
behaviors?: Behavior[];
/** Chart display settings related to current view context */
Expand Down Expand Up @@ -128,6 +137,8 @@ export default class ChartProps<FormData extends RawFormData = RawFormData> {

filterState: FilterState;

legendState?: LegendState;

queriesData: QueryData[];

width: number;
Expand Down Expand Up @@ -156,6 +167,7 @@ export default class ChartProps<FormData extends RawFormData = RawFormData> {
hooks = {},
ownState = {},
filterState = {},
legendState,
initialValues = {},
queriesData = [],
behaviors = [],
Expand All @@ -181,6 +193,7 @@ export default class ChartProps<FormData extends RawFormData = RawFormData> {
this.queriesData = queriesData;
this.ownState = ownState;
this.filterState = filterState;
this.legendState = legendState;
this.behaviors = behaviors;
this.displaySettings = displaySettings;
this.appSection = appSection;
Expand All @@ -205,6 +218,7 @@ ChartProps.createSelector = function create(): ChartPropsSelector {
input => input.width,
input => input.ownState,
input => input.filterState,
input => input.legendState,
input => input.behaviors,
input => input.displaySettings,
input => input.appSection,
Expand All @@ -224,6 +238,7 @@ ChartProps.createSelector = function create(): ChartPropsSelector {
width,
ownState,
filterState,
legendState,
behaviors,
displaySettings,
appSection,
Expand All @@ -243,6 +258,7 @@ ChartProps.createSelector = function create(): ChartPropsSelector {
queriesData,
ownState,
filterState,
legendState,
width,
behaviors,
displaySettings,
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/packages/superset-ui-core/src/chart/types/Base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,8 @@ export enum AxisType {
log = 'log',
}

export interface LegendState {
[key: string]: boolean;
}

export default {};
8 changes: 1 addition & 7 deletions ...rset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
import { EchartsMixedTimeseriesChartTransformedProps } from './types';
import Echart from '../components/Echart';
import { EventHandlers } from '../types';
import { currentSeries, formatSeriesName } from '../utils/series';
import { formatSeriesName } from '../utils/series';

export default function EchartsMixedTimeseries({
height,
Expand Down Expand Up @@ -123,12 +123,6 @@ export default function EchartsMixedTimeseries({
const { seriesName, seriesIndex } = props;
handleChange(seriesName, seriesIndex);
},
mouseout: () => {
currentSeries.name = '';
},
mouseover: params => {
currentSeries.name = params.seriesName;
michael-s-molina marked this conversation as resolved.
Show resolved Hide resolved
},
contextmenu: async eventParams => {
if (onContextMenu) {
eventParams.event.stop();
Expand Down
7 changes: 1 addition & 6 deletions superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ import {
import { parseYAxisBound } from '../utils/controls';
import {
getOverMaxHiddenFormatter,
currentSeries,
dedupSeries,
extractSeries,
getAxisType,
Expand Down Expand Up @@ -481,11 +480,7 @@ export default function transformProps(
seriesName: key,
formatter: primarySeries.has(key) ? formatter : formatterSecondary,
});
if (currentSeries.name === key) {
rows.push(`<span style="font-weight: 700">${content}</span>`);
} else {
rows.push(`<span style="opacity: 0.7">${content}</span>`);
}
rows.push(`<span style="opacity: 0.7">${content}</span>`);
});
return rows.join('<br />');
},
Expand Down
79 changes: 18 additions & 61 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,19 @@ import {
getTimeFormatter,
getColumnLabel,
getNumberFormatter,
LegendState,
} from '@superset-ui/core';
import { ViewRootGroup } from 'echarts/types/src/util/types';
import GlobalModel from 'echarts/types/src/model/Global';
import ComponentModel from 'echarts/types/src/model/Component';
import { EchartsHandler, EventHandlers } from '../types';
import Echart from '../components/Echart';
import { TimeseriesChartTransformedProps } from './types';
import { currentSeries, formatSeriesName } from '../utils/series';
import { formatSeriesName } from '../utils/series';
import { ExtraControls } from '../components/ExtraControls';

const TIMER_DURATION = 300;

// @ts-ignore
export default function EchartsTimeseries({
formData,
height,
Expand All @@ -49,6 +49,7 @@ export default function EchartsTimeseries({
setControlValue,
legendData = [],
onContextMenu,
onLegendStateChanged,
xValueFormatter,
xAxis,
refs,
Expand All @@ -59,8 +60,6 @@ export default function EchartsTimeseries({
const echartRef = useRef<EchartsHandler | null>(null);
// eslint-disable-next-line no-param-reassign
refs.echartRef = echartRef;
const lastTimeRef = useRef(Date.now());
const lastSelectedLegend = useRef('');
const clickTimer = useRef<ReturnType<typeof setTimeout>>();
const extraControlRef = useRef<HTMLDivElement>(null);
const [extraControlHeight, setExtraControlHeight] = useState(0);
Expand All @@ -69,34 +68,6 @@ export default function EchartsTimeseries({
setExtraControlHeight(updatedHeight);
}, [formData.showExtraControls]);

const handleDoubleClickChange = useCallback(
(name?: string) => {
const echartInstance = echartRef.current?.getEchartInstance();
if (!name) {
currentSeries.legend = '';
echartInstance?.dispatchAction({
type: 'legendAllSelect',
});
} else {
legendData.forEach(datum => {
if (datum === name) {
currentSeries.legend = datum;
echartInstance?.dispatchAction({
type: 'legendSelect',
name: datum,
});
} else {
echartInstance?.dispatchAction({
type: 'legendUnSelect',
name: datum,
});
}
});
}
},
[legendData],
);

const getModelInfo = (target: ViewRootGroup, globalModel: GlobalModel) => {
let el = target;
let model: ComponentModel | null = null;
Expand Down Expand Up @@ -175,30 +146,14 @@ export default function EchartsTimeseries({
handleChange(name);
}, TIMER_DURATION);
},
mouseout: () => {
currentSeries.name = '';
legendselectchanged: payload => {
onLegendStateChanged?.(payload.selected);
},
mouseover: params => {
currentSeries.name = params.seriesName;
legendselectall: payload => {
onLegendStateChanged?.(payload.selected);
},
legendselectchanged: payload => {
const currentTime = Date.now();
// TIMER_DURATION is the interval between two legendselectchanged event
if (
currentTime - lastTimeRef.current < TIMER_DURATION &&
lastSelectedLegend.current === payload.name
) {
// execute dbclick
handleDoubleClickChange(payload.name);
} else {
lastTimeRef.current = currentTime;
// remember last selected legend
lastSelectedLegend.current = payload.name;
}
// if all legend is unselected, we keep all selected
if (Object.values(payload.selected).every(i => !i)) {
handleDoubleClickChange();
}
legendinverseselect: payload => {
onLegendStateChanged?.(payload.selected);
},
contextmenu: async eventParams => {
if (onContextMenu) {
Expand Down Expand Up @@ -272,15 +227,16 @@ export default function EchartsTimeseries({
// @ts-ignore
const globalModel = echartInstance.getModel();
const model = getModelInfo(params.target, globalModel);
const seriesCount = globalModel.getSeriesCount();
const currentSeriesIndices = globalModel.getCurrentSeriesIndices();
if (model) {
const { name } = model;
if (seriesCount !== currentSeriesIndices.length) {
handleDoubleClickChange();
} else {
handleDoubleClickChange(name);
}
const legendState: LegendState = legendData.reduce(
(previous, datum) => ({
...previous,
[datum]: datum === name,
}),
{},
);
onLegendStateChanged?.(legendState);
}
}
},
Expand All @@ -292,6 +248,7 @@ export default function EchartsTimeseries({
<ExtraControls formData={formData} setControlValue={setControlValue} />
</div>
<Echart
ref={echartRef}
refs={refs}
height={height - extraControlHeight}
width={width}
Expand Down
10 changes: 8 additions & 2 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ import { ForecastSeriesEnum, ForecastValue, Refs } from '../types';
import { parseYAxisBound } from '../utils/controls';
import {
calculateLowerLogTick,
currentSeries,
dedupSeries,
extractDataTotalValues,
extractSeries,
Expand Down Expand Up @@ -101,6 +100,7 @@ export default function transformProps(
width,
height,
filterState,
legendState,
formData,
hooks,
queriesData,
Expand Down Expand Up @@ -192,6 +192,7 @@ export default function transformProps(
stack,
percentageThreshold,
xAxisCol: xAxisLabel,
legendState,
},
);
const extraMetricLabels = extractExtraMetrics(chartProps.rawFormData).map(
Expand Down Expand Up @@ -221,6 +222,7 @@ export default function transformProps(
stack,
onlyTotal,
isHorizontal,
legendState,
});
const seriesContexts = extractForecastSeriesContexts(
Object.values(rawSeries).map(series => series.name as string),
Expand Down Expand Up @@ -258,6 +260,7 @@ export default function transformProps(
markerSize,
areaOpacity: opacity,
seriesType,
legendState,
stack,
formatter,
showValue,
Expand Down Expand Up @@ -379,6 +382,7 @@ export default function transformProps(
setDataMask = () => {},
setControlValue = () => {},
onContextMenu,
onLegendStateChanged,
} = hooks;

const addYAxisLabelOffset = !!yAxisTitle;
Expand Down Expand Up @@ -486,7 +490,7 @@ export default function transformProps(
seriesName: key,
formatter,
});
if (currentSeries.name === key) {
if (!legendState || legendState[key]) {
rows.push(`<span style="font-weight: 700">${content}</span>`);
} else {
rows.push(`<span style="opacity: 0.7">${content}</span>`);
Expand All @@ -506,6 +510,7 @@ export default function transformProps(
showLegend,
theme,
zoomable,
legendState,
),
data: legendData as string[],
},
Expand Down Expand Up @@ -549,6 +554,7 @@ export default function transformProps(
width,
legendData,
onContextMenu,
onLegendStateChanged,
xValueFormatter: tooltipFormatter,
xAxis: {
label: xAxisLabel,
Expand Down
15 changes: 11 additions & 4 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
getTimeFormatter,
IntervalAnnotationLayer,
isTimeseriesAnnotationResult,
LegendState,
NumberFormatter,
smartDateDetailedFormatter,
smartDateFormatter,
Expand Down Expand Up @@ -65,7 +66,7 @@ import {
formatAnnotationLabel,
parseAnnotationOpacity,
} from '../utils/annotation';
import { currentSeries, getChartPadding } from '../utils/series';
import { getChartPadding } from '../utils/series';
import {
OpacityEnum,
StackControlsValue,
Expand Down Expand Up @@ -156,6 +157,7 @@ export function transformSeries(
yAxisIndex?: number;
showValue?: boolean;
onlyTotal?: boolean;
legendState?: LegendState;
formatter?: NumberFormatter;
totalStackedValues?: number[];
showValueIndexes?: number[];
Expand All @@ -182,6 +184,7 @@ export function transformSeries(
showValue,
onlyTotal,
formatter,
legendState,
totalStackedValues = [],
showValueIndexes = [],
thresholdValues = [],
Expand Down Expand Up @@ -308,10 +311,14 @@ export function transformSeries(
formatter: (params: any) => {
const { value, dataIndex, seriesIndex, seriesName } = params;
const numericValue = isHorizontal ? value[0] : value[1];
const isSelectedLegend = currentSeries.legend === seriesName;
const isSelectedLegend = !legendState || legendState[seriesName];
const isAreaExpand = stack === StackControlsValue.Expand;
if (!formatter) return numericValue;
if (!stack || isSelectedLegend) return formatter(numericValue);
if (!formatter) {
return numericValue;
}
if (!stack && isSelectedLegend) {
return formatter(numericValue);
}
if (!onlyTotal) {
if (
numericValue >=
Expand Down
Loading