From c281cbe71a3f9897f8480215b69cbe0f71bf88db Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Tue, 14 Dec 2021 17:04:14 -0700 Subject: [PATCH 1/9] feat(timeseries-chart): add percentage threshold control for stack series labels --- .../Timeseries/Stories.tsx | 3 +++ .../src/Timeseries/transformProps.ts | 2 ++ .../src/Timeseries/transformers.ts | 12 ++++++++++-- .../plugin-chart-echarts/src/controls.tsx | 19 +++++++++++++++++++ 4 files changed, 34 insertions(+), 2 deletions(-) diff --git a/superset-frontend/packages/superset-ui-demo/storybook/stories/plugins/plugin-chart-echarts/Timeseries/Stories.tsx b/superset-frontend/packages/superset-ui-demo/storybook/stories/plugins/plugin-chart-echarts/Timeseries/Stories.tsx index 3c20a76c06345..3f219ed6e66ab 100644 --- a/superset-frontend/packages/superset-ui-demo/storybook/stories/plugins/plugin-chart-echarts/Timeseries/Stories.tsx +++ b/superset-frontend/packages/superset-ui-demo/storybook/stories/plugins/plugin-chart-echarts/Timeseries/Stories.tsx @@ -74,6 +74,9 @@ export const Timeseries = ({ width, height }) => { logAxis: boolean('Log axis', false), yAxisFormat: 'SMART_NUMBER', stack: boolean('Stack', false), + showValue: boolean('Show Values', false), + onlyTotal: boolean('Only Total', false), + percentageThreshold: number('Percentage Threshold', 0), area: boolean('Area chart', false), markerEnabled: boolean('Enable markers', false), markerSize: number('Marker Size', 6), diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index b63feb35737c5..3fee96a45db79 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -111,6 +111,7 @@ export default function transformProps( groupby, showValue, onlyTotal, + percentageThreshold, xAxisTitle, yAxisTitle, xAxisTitleMargin, @@ -166,6 +167,7 @@ export default function transformProps( formatter, showValue, onlyTotal, + percentageThreshold, totalStackedValues, showValueIndexes, richTooltip, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts index 3ca56006adce7..8a67482a3e997 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -78,6 +78,7 @@ export function transformSeries( yAxisIndex?: number; showValue?: boolean; onlyTotal?: boolean; + percentageThreshold?: number; formatter?: NumberFormatter; totalStackedValues?: number[]; showValueIndexes?: number[]; @@ -97,6 +98,7 @@ export function transformSeries( yAxisIndex = 0, showValue, onlyTotal, + percentageThreshold = 0, formatter, totalStackedValues = [], showValueIndexes = [], @@ -211,8 +213,14 @@ export function transformSeries( } = params; const isSelectedLegend = currentSeries.legend === seriesName; if (!formatter) return numericValue; - if (!stack || !onlyTotal || isSelectedLegend) { - return formatter(numericValue); + if (!stack || isSelectedLegend) return formatter(numericValue); + if (!onlyTotal) { + if ( + numericValue >= + (percentageThreshold / 100) * totalStackedValues[dataIndex] + ) { + return formatter(numericValue); + } } if (seriesIndex === showValueIndexes[dataIndex]) { return formatter(totalStackedValues[dataIndex]); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx index 76c7c42186ffc..f1b89b26646c5 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx @@ -136,10 +136,29 @@ const onlyTotalControl = { }, }; +const percentageThresholdControl = { + name: 'percentage_threshold', + config: { + type: 'TextControl', + label: t('Percentage threshold'), + renderTrigger: true, + isFloat: true, + default: 5, + description: t( + 'Minimum threshold in percentage points for showing labels.', + ), + visibility: ({ controls }: ControlPanelsContainerProps) => + Boolean(controls?.show_value?.value) && + Boolean(controls?.stack?.value) && + Boolean(!controls?.only_total?.value), + }, +}; + export const showValueSection = [ [showValueControl], [stackControl], [onlyTotalControl], + [percentageThresholdControl], ]; const richTooltipControl = { From 21d04d62fac2327a068cdc78230e23a2a13dc477 Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Tue, 14 Dec 2021 20:50:48 -0700 Subject: [PATCH 2/9] feat: move threshold vlues to an array --- .../src/Timeseries/transformProps.ts | 4 +++- .../plugin-chart-echarts/src/Timeseries/transformers.ts | 9 +++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 3fee96a45db79..6f275a0680a24 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -131,6 +131,7 @@ export default function transformProps( const totalStackedValues: number[] = []; const showValueIndexes: number[] = []; + const thresholdValues: number[] = []; rebasedData.forEach(data => { const values = Object.keys(data).reduce((prev, curr) => { @@ -141,6 +142,7 @@ export default function transformProps( return prev + (value as number); }, 0); totalStackedValues.push(values); + thresholdValues.push((percentageThreshold / 100) * values); }); if (stack) { @@ -167,9 +169,9 @@ export default function transformProps( formatter, showValue, onlyTotal, - percentageThreshold, totalStackedValues, showValueIndexes, + thresholdValues, richTooltip, }); if (transformedSeries) series.push(transformedSeries); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts index 8a67482a3e997..b05d50ee574d2 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -78,10 +78,10 @@ export function transformSeries( yAxisIndex?: number; showValue?: boolean; onlyTotal?: boolean; - percentageThreshold?: number; formatter?: NumberFormatter; totalStackedValues?: number[]; showValueIndexes?: number[]; + thresholdValues?: number[]; richTooltip?: boolean; }, ): SeriesOption | undefined { @@ -98,10 +98,10 @@ export function transformSeries( yAxisIndex = 0, showValue, onlyTotal, - percentageThreshold = 0, formatter, totalStackedValues = [], showValueIndexes = [], + thresholdValues = [], richTooltip, } = opts; const contexts = seriesContexts[name || ''] || []; @@ -215,10 +215,7 @@ export function transformSeries( if (!formatter) return numericValue; if (!stack || isSelectedLegend) return formatter(numericValue); if (!onlyTotal) { - if ( - numericValue >= - (percentageThreshold / 100) * totalStackedValues[dataIndex] - ) { + if (numericValue >= thresholdValues[dataIndex]) { return formatter(numericValue); } } From 03a668bb8393f5ecf4fde9b417fcbcd43d00c9c9 Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Wed, 15 Dec 2021 20:26:11 -0700 Subject: [PATCH 3/9] add tests for showValue, onlyTotal, and percentThreshold --- .../src/Timeseries/transformers.ts | 1 + .../src/Timeseries/types.ts | 2 + .../test/Timeseries/transformProps.test.ts | 215 ++++++++++++++++-- 3 files changed, 200 insertions(+), 18 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts index b05d50ee574d2..d6cbcdc77d274 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -218,6 +218,7 @@ export function transformSeries( if (numericValue >= thresholdValues[dataIndex]) { return formatter(numericValue); } + return ''; } if (seriesIndex === showValueIndexes[dataIndex]) { return formatter(totalStackedValues[dataIndex]); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts index ac634239229de..82287aa5e6a71 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts @@ -81,6 +81,7 @@ export type EchartsTimeseriesFormData = QueryFormData & { groupby: QueryFormColumn[]; showValue: boolean; onlyTotal: boolean; + percentThreshold: number; } & EchartsLegendFormData & EchartsTitleFormData; @@ -117,6 +118,7 @@ export const DEFAULT_FORM_DATA: EchartsTimeseriesFormData = { groupby: [], showValue: false, onlyTotal: false, + percentThreshold: 0, ...DEFAULT_TITLE_FORM_DATA, }; diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts index 96c5865d76c66..b891100040568 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts @@ -22,10 +22,14 @@ import { FormulaAnnotationLayer, IntervalAnnotationLayer, TimeseriesAnnotationLayer, + AnnotationStyle, + AnnotationType, + AnnotationSourceType, } from '@superset-ui/core'; import transformProps from '../../src/Timeseries/transformProps'; +import { EchartsTimeseriesChartProps } from '../../src/Timeseries/types'; -describe('EchartsTimeseries tranformProps', () => { +describe('EchartsTimeseries transformProps', () => { const formData = { colorScheme: 'bnbColors', datasource: '3__table', @@ -49,8 +53,8 @@ describe('EchartsTimeseries tranformProps', () => { }; it('should tranform chart props for viz', () => { - const chartProps = new ChartProps(chartPropsConfig); - expect(transformProps(chartProps)).toEqual( + const chartProps: unknown = new ChartProps(chartPropsConfig); + expect(transformProps(chartProps as EchartsTimeseriesChartProps)).toEqual( expect.objectContaining({ width: 800, height: 600, @@ -82,19 +86,20 @@ describe('EchartsTimeseries tranformProps', () => { it('should add a formula annotation to viz', () => { const formula: FormulaAnnotationLayer = { name: 'My Formula', - annotationType: 'FORMULA', + annotationType: AnnotationType.Formula, value: 'x+1', - style: 'solid', + style: AnnotationStyle.Solid, + showLabel: true, show: true, }; - const chartProps = new ChartProps({ + const chartProps: unknown = new ChartProps({ ...chartPropsConfig, formData: { ...formData, annotationLayers: [formula], }, }); - expect(transformProps(chartProps)).toEqual( + expect(transformProps(chartProps as EchartsTimeseriesChartProps)).toEqual( expect.objectContaining({ width: 800, height: 600, @@ -132,37 +137,40 @@ describe('EchartsTimeseries tranformProps', () => { it('should add an interval, event and timeseries annotation to viz', () => { const event: EventAnnotationLayer = { - annotationType: 'EVENT', + annotationType: AnnotationType.Event, name: 'My Event', show: true, - sourceType: 'NATIVE', - style: 'solid', + showLabel: true, + sourceType: AnnotationSourceType.Native, + style: AnnotationStyle.Solid, value: 1, }; const interval: IntervalAnnotationLayer = { - annotationType: 'INTERVAL', + annotationType: AnnotationType.Interval, name: 'My Interval', show: true, - sourceType: 'table', + showLabel: true, + sourceType: AnnotationSourceType.Table, titleColumn: '', timeColumn: 'start', intervalEndColumn: '', descriptionColumns: [], - style: 'dashed', + style: AnnotationStyle.Dashed, value: 2, }; const timeseries: TimeseriesAnnotationLayer = { - annotationType: 'TIME_SERIES', + annotationType: AnnotationType.Timeseries, name: 'My Timeseries', show: true, - sourceType: 'line', - style: 'solid', + showLabel: true, + sourceType: AnnotationSourceType.Line, + style: AnnotationStyle.Solid, titleColumn: '', value: 3, }; - const chartProps = new ChartProps({ + const chartProps: unknown = new ChartProps({ ...chartPropsConfig, formData: { ...formData, @@ -219,7 +227,7 @@ describe('EchartsTimeseries tranformProps', () => { }, ], }); - expect(transformProps(chartProps)).toEqual( + expect(transformProps(chartProps as EchartsTimeseriesChartProps)).toEqual( expect.objectContaining({ echartOptions: expect.objectContaining({ legend: expect.objectContaining({ @@ -244,3 +252,174 @@ describe('EchartsTimeseries tranformProps', () => { ); }); }); + +describe('Does transformProps transform series correctly', () => { + type seriesDataType = [Date, number]; + type labelFormatterType = (params: { + value: seriesDataType; + dataIndex: number; + seriesIndex: number; + }) => string; + type seriesType = { + label: { show: boolean; formatter: labelFormatterType }; + data: seriesDataType[]; + name: string; + }; + + const formData = { + colorScheme: 'bnbColors', + datasource: '3__table', + granularity_sqla: 'ds', + metric: 'sum__num', + groupby: ['foo', 'bar'], + showValue: true, + stack: true, + onlyTotal: false, + percentageThreshold: 50, + }; + const queriesData = [ + { + data: [ + { + 'San Francisco': 1, + 'New York': 2, + Boston: 1, + __timestamp: 599616000000, + }, + { + 'San Francisco': 3, + 'New York': 4, + Boston: 1, + __timestamp: 599916000000, + }, + { + 'San Francisco': 5, + 'New York': 8, + Boston: 6, + __timestamp: 600216000000, + }, + { + 'San Francisco': 2, + 'New York': 7, + Boston: 2, + __timestamp: 600516000000, + }, + ], + }, + ]; + const chartPropsConfig = { + formData, + width: 800, + height: 600, + queriesData, + }; + + const stackTotals = queriesData[0].data.reduce((totals, currentStack) => { + const total = Object.keys(currentStack).reduce((stackSum, key) => { + if (key === '__timestamp') return stackSum; + return stackSum + currentStack[key]; + }, 0); + totals.push(total); + return totals; + }, [] as number[]); + + it('should show labels when showValue is true', () => { + const chartProps: unknown = new ChartProps(chartPropsConfig); + + const transformedSeries = transformProps( + chartProps as EchartsTimeseriesChartProps, + ).echartOptions.series as seriesType[]; + + transformedSeries.forEach(series => { + expect(series.label.show).toBe(true); + }); + }); + + it('should not show labels when showValue is false', () => { + const updatedChartPropsConfig = { + ...chartPropsConfig, + formData: { ...chartPropsConfig.formData, showValue: false }, + }; + + const chartProps: unknown = new ChartProps(updatedChartPropsConfig); + + const transformedSeries = transformProps( + chartProps as EchartsTimeseriesChartProps, + ).echartOptions.series as seriesType[]; + + transformedSeries.forEach(series => { + expect(series.label.show).toBe(false); + }); + }); + + it('should show only totals when onlyTotal is true', () => { + const updatedChartPropsConfig = { + ...chartPropsConfig, + formData: { ...chartPropsConfig.formData, onlyTotal: true }, + }; + + const chartProps: unknown = new ChartProps(updatedChartPropsConfig); + + const transformedSeries = transformProps( + chartProps as EchartsTimeseriesChartProps, + ).echartOptions.series as seriesType[]; + + const showValueIndexes: number[] = []; + + transformedSeries.forEach((entry, seriesIndex) => { + const { data = [] } = entry; + (data as [Date, number][]).forEach((datum, dataIndex) => { + if (datum[1] !== null) { + showValueIndexes[dataIndex] = seriesIndex; + } + }); + }); + + transformedSeries.forEach((series, seriesIndex) => { + expect(series.label.show).toBe(true); + series.data.forEach((value, dataIndex) => { + const params = { + value, + dataIndex, + seriesIndex, + }; + + let expectedLabel: string; + + if (seriesIndex === showValueIndexes[dataIndex]) { + expectedLabel = String(stackTotals[dataIndex]); + } else { + expectedLabel = ''; + } + + expect(series.label.formatter(params)).toBe(expectedLabel); + }); + }); + }); + + it('should show labels on values >= percentageThreshold if onlyTotal false', () => { + const chartProps: unknown = new ChartProps(chartPropsConfig); + + const transformedSeries = transformProps( + chartProps as EchartsTimeseriesChartProps, + ).echartOptions.series as seriesType[]; + + const expectedThresholds = stackTotals.map( + total => (chartPropsConfig.formData.percentageThreshold / 100) * total, + ); + + transformedSeries.forEach((series, seriesIndex) => { + expect(series.label.show).toBe(true); + series.data.forEach((value, dataIndex) => { + const params = { + value, + dataIndex, + seriesIndex, + }; + const expectedLabel = + value[1] >= expectedThresholds[dataIndex] ? String(value[1]) : ''; + expect(series.label.formatter(params)).toBe(expectedLabel); + }); + }); + }); +}); From 6f8927209b80dc61ba14fc66ff94871b36a83321 Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Thu, 16 Dec 2021 09:34:45 -0700 Subject: [PATCH 4/9] feat: add another test --- .../test/Timeseries/transformProps.test.ts | 57 ++++++++++++++----- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts index b891100040568..f8a2be0f1511a 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts @@ -314,14 +314,17 @@ describe('Does transformProps transform series correctly', () => { queriesData, }; - const stackTotals = queriesData[0].data.reduce((totals, currentStack) => { - const total = Object.keys(currentStack).reduce((stackSum, key) => { - if (key === '__timestamp') return stackSum; - return stackSum + currentStack[key]; - }, 0); - totals.push(total); - return totals; - }, [] as number[]); + const totalStackedValues = queriesData[0].data.reduce( + (totals, currentStack) => { + const total = Object.keys(currentStack).reduce((stackSum, key) => { + if (key === '__timestamp') return stackSum; + return stackSum + currentStack[key]; + }, 0); + totals.push(total); + return totals; + }, + [] as number[], + ); it('should show labels when showValue is true', () => { const chartProps: unknown = new ChartProps(chartPropsConfig); @@ -338,7 +341,7 @@ describe('Does transformProps transform series correctly', () => { it('should not show labels when showValue is false', () => { const updatedChartPropsConfig = { ...chartPropsConfig, - formData: { ...chartPropsConfig.formData, showValue: false }, + formData: { ...formData, showValue: false }, }; const chartProps: unknown = new ChartProps(updatedChartPropsConfig); @@ -355,7 +358,7 @@ describe('Does transformProps transform series correctly', () => { it('should show only totals when onlyTotal is true', () => { const updatedChartPropsConfig = { ...chartPropsConfig, - formData: { ...chartPropsConfig.formData, onlyTotal: true }, + formData: { ...formData, onlyTotal: true }, }; const chartProps: unknown = new ChartProps(updatedChartPropsConfig); @@ -387,7 +390,7 @@ describe('Does transformProps transform series correctly', () => { let expectedLabel: string; if (seriesIndex === showValueIndexes[dataIndex]) { - expectedLabel = String(stackTotals[dataIndex]); + expectedLabel = String(totalStackedValues[dataIndex]); } else { expectedLabel = ''; } @@ -397,15 +400,15 @@ describe('Does transformProps transform series correctly', () => { }); }); - it('should show labels on values >= percentageThreshold if onlyTotal false', () => { + it('should show labels on values >= percentageThreshold if onlyTotal is false', () => { const chartProps: unknown = new ChartProps(chartPropsConfig); const transformedSeries = transformProps( chartProps as EchartsTimeseriesChartProps, ).echartOptions.series as seriesType[]; - const expectedThresholds = stackTotals.map( - total => (chartPropsConfig.formData.percentageThreshold / 100) * total, + const expectedThresholds = totalStackedValues.map( + total => (formData.percentageThreshold / 100) * total, ); transformedSeries.forEach((series, seriesIndex) => { @@ -422,4 +425,30 @@ describe('Does transformProps transform series correctly', () => { }); }); }); + + it('should not apply percentage threshold when showValue is true and stack is false', () => { + const updatedChartPropsConfig = { + ...chartPropsConfig, + formData: { ...formData, stack: false }, + }; + + const chartProps: unknown = new ChartProps(updatedChartPropsConfig); + + const transformedSeries = transformProps( + chartProps as EchartsTimeseriesChartProps, + ).echartOptions.series as seriesType[]; + + transformedSeries.forEach((series, seriesIndex) => { + expect(series.label.show).toBe(true); + series.data.forEach((value, dataIndex) => { + const params = { + value, + dataIndex, + seriesIndex, + }; + const expectedLabel = String(value[1]); + expect(series.label.formatter(params)).toBe(expectedLabel); + }); + }); + }); }); From 1b3880f27838a9a09c20636594dbce03375ee4ff Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Mon, 20 Dec 2021 08:13:04 -0700 Subject: [PATCH 5/9] revert ChartProps typesetting, fix misnamed variable on form data type, and other minor changes --- .../src/Timeseries/transformProps.ts | 2 +- .../src/Timeseries/types.ts | 4 +- .../test/Timeseries/transformProps.test.ts | 50 ++++++++----------- 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 6f275a0680a24..31316d2c84521 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -142,7 +142,7 @@ export default function transformProps( return prev + (value as number); }, 0); totalStackedValues.push(values); - thresholdValues.push((percentageThreshold / 100) * values); + thresholdValues.push((percentageThreshold || 0 / 100) * values); }); if (stack) { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts index 82287aa5e6a71..9b32b6bf704c0 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts @@ -81,7 +81,7 @@ export type EchartsTimeseriesFormData = QueryFormData & { groupby: QueryFormColumn[]; showValue: boolean; onlyTotal: boolean; - percentThreshold: number; + percentageThreshold: number; } & EchartsLegendFormData & EchartsTitleFormData; @@ -118,7 +118,7 @@ export const DEFAULT_FORM_DATA: EchartsTimeseriesFormData = { groupby: [], showValue: false, onlyTotal: false, - percentThreshold: 0, + percentageThreshold: 0, ...DEFAULT_TITLE_FORM_DATA, }; diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts index f8a2be0f1511a..748ec1b5392c6 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts @@ -27,7 +27,6 @@ import { AnnotationSourceType, } from '@superset-ui/core'; import transformProps from '../../src/Timeseries/transformProps'; -import { EchartsTimeseriesChartProps } from '../../src/Timeseries/types'; describe('EchartsTimeseries transformProps', () => { const formData = { @@ -53,8 +52,8 @@ describe('EchartsTimeseries transformProps', () => { }; it('should tranform chart props for viz', () => { - const chartProps: unknown = new ChartProps(chartPropsConfig); - expect(transformProps(chartProps as EchartsTimeseriesChartProps)).toEqual( + const chartProps = new ChartProps(chartPropsConfig); + expect(transformProps(chartProps)).toEqual( expect.objectContaining({ width: 800, height: 600, @@ -92,14 +91,14 @@ describe('EchartsTimeseries transformProps', () => { showLabel: true, show: true, }; - const chartProps: unknown = new ChartProps({ + const chartProps = new ChartProps({ ...chartPropsConfig, formData: { ...formData, annotationLayers: [formula], }, }); - expect(transformProps(chartProps as EchartsTimeseriesChartProps)).toEqual( + expect(transformProps(chartProps)).toEqual( expect.objectContaining({ width: 800, height: 600, @@ -170,7 +169,7 @@ describe('EchartsTimeseries transformProps', () => { titleColumn: '', value: 3, }; - const chartProps: unknown = new ChartProps({ + const chartProps = new ChartProps({ ...chartPropsConfig, formData: { ...formData, @@ -227,7 +226,7 @@ describe('EchartsTimeseries transformProps', () => { }, ], }); - expect(transformProps(chartProps as EchartsTimeseriesChartProps)).toEqual( + expect(transformProps(chartProps)).toEqual( expect.objectContaining({ echartOptions: expect.objectContaining({ legend: expect.objectContaining({ @@ -327,11 +326,10 @@ describe('Does transformProps transform series correctly', () => { ); it('should show labels when showValue is true', () => { - const chartProps: unknown = new ChartProps(chartPropsConfig); + const chartProps = new ChartProps(chartPropsConfig); - const transformedSeries = transformProps( - chartProps as EchartsTimeseriesChartProps, - ).echartOptions.series as seriesType[]; + const transformedSeries = transformProps(chartProps).echartOptions + .series as seriesType[]; transformedSeries.forEach(series => { expect(series.label.show).toBe(true); @@ -344,11 +342,10 @@ describe('Does transformProps transform series correctly', () => { formData: { ...formData, showValue: false }, }; - const chartProps: unknown = new ChartProps(updatedChartPropsConfig); + const chartProps = new ChartProps(updatedChartPropsConfig); - const transformedSeries = transformProps( - chartProps as EchartsTimeseriesChartProps, - ).echartOptions.series as seriesType[]; + const transformedSeries = transformProps(chartProps).echartOptions + .series as seriesType[]; transformedSeries.forEach(series => { expect(series.label.show).toBe(false); @@ -361,11 +358,10 @@ describe('Does transformProps transform series correctly', () => { formData: { ...formData, onlyTotal: true }, }; - const chartProps: unknown = new ChartProps(updatedChartPropsConfig); + const chartProps = new ChartProps(updatedChartPropsConfig); - const transformedSeries = transformProps( - chartProps as EchartsTimeseriesChartProps, - ).echartOptions.series as seriesType[]; + const transformedSeries = transformProps(chartProps).echartOptions + .series as seriesType[]; const showValueIndexes: number[] = []; @@ -401,14 +397,13 @@ describe('Does transformProps transform series correctly', () => { }); it('should show labels on values >= percentageThreshold if onlyTotal is false', () => { - const chartProps: unknown = new ChartProps(chartPropsConfig); + const chartProps = new ChartProps(chartPropsConfig); - const transformedSeries = transformProps( - chartProps as EchartsTimeseriesChartProps, - ).echartOptions.series as seriesType[]; + const transformedSeries = transformProps(chartProps).echartOptions + .series as seriesType[]; const expectedThresholds = totalStackedValues.map( - total => (formData.percentageThreshold / 100) * total, + total => (formData.percentageThreshold || 0 / 100) * total, ); transformedSeries.forEach((series, seriesIndex) => { @@ -432,11 +427,10 @@ describe('Does transformProps transform series correctly', () => { formData: { ...formData, stack: false }, }; - const chartProps: unknown = new ChartProps(updatedChartPropsConfig); + const chartProps = new ChartProps(updatedChartPropsConfig); - const transformedSeries = transformProps( - chartProps as EchartsTimeseriesChartProps, - ).echartOptions.series as seriesType[]; + const transformedSeries = transformProps(chartProps).echartOptions + .series as seriesType[]; transformedSeries.forEach((series, seriesIndex) => { expect(series.label.show).toBe(true); From dfc2b3631430c3e44241204c3bf0f7276a97a850 Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Mon, 20 Dec 2021 09:28:48 -0700 Subject: [PATCH 6/9] fix percentage threshold push equation --- .../plugin-chart-echarts/src/Timeseries/transformProps.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 31316d2c84521..44f97e9bac020 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -142,7 +142,7 @@ export default function transformProps( return prev + (value as number); }, 0); totalStackedValues.push(values); - thresholdValues.push((percentageThreshold || 0 / 100) * values); + thresholdValues.push(((percentageThreshold || 0) / 100) * values); }); if (stack) { From 5bc821fcd1079cb356b50be893413e04072587b4 Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Mon, 20 Dec 2021 11:27:10 -0700 Subject: [PATCH 7/9] fix percentage threshold push equation in tests --- .../plugin-chart-echarts/test/Timeseries/transformProps.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts index 748ec1b5392c6..ab82417f5492f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts @@ -403,7 +403,7 @@ describe('Does transformProps transform series correctly', () => { .series as seriesType[]; const expectedThresholds = totalStackedValues.map( - total => (formData.percentageThreshold || 0 / 100) * total, + total => ((formData.percentageThreshold || 0) / 100) * total, ); transformedSeries.forEach((series, seriesIndex) => { From 4f1f03f0e03b1d3f83675087a350de548744512a Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Tue, 11 Jan 2022 13:20:36 -0700 Subject: [PATCH 8/9] change default on control to match form --- .../plugins/plugin-chart-echarts/src/controls.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx index f1b89b26646c5..fdac4d4220d5a 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx @@ -24,6 +24,7 @@ import { sharedControls, } from '@superset-ui/chart-controls'; import { DEFAULT_LEGEND_FORM_DATA } from './types'; +import { DEFAULT_FORM_DATA } from '.'; const { legendMargin, legendOrientation, legendType, showLegend } = DEFAULT_LEGEND_FORM_DATA; @@ -143,7 +144,7 @@ const percentageThresholdControl = { label: t('Percentage threshold'), renderTrigger: true, isFloat: true, - default: 5, + default: DEFAULT_FORM_DATA.percentageThreshold, description: t( 'Minimum threshold in percentage points for showing labels.', ), From 814bff0fc15c076bdacc20cad358357179dc5b59 Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Tue, 11 Jan 2022 14:21:18 -0700 Subject: [PATCH 9/9] attempt fix form defaults import --- superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx index fdac4d4220d5a..614a0481e1951 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx @@ -24,7 +24,7 @@ import { sharedControls, } from '@superset-ui/chart-controls'; import { DEFAULT_LEGEND_FORM_DATA } from './types'; -import { DEFAULT_FORM_DATA } from '.'; +import { DEFAULT_FORM_DATA } from './Timeseries/types'; const { legendMargin, legendOrientation, legendType, showLegend } = DEFAULT_LEGEND_FORM_DATA;