From a8ba544e609ad3af449239c1fb956bb18c7066c4 Mon Sep 17 00:00:00 2001 From: "JUST.in DO IT" Date: Thu, 11 Aug 2022 11:28:18 -0700 Subject: [PATCH] fix(plugin-chart-echarts): invalid total label location for negative values in stacked bar chart (#21032) --- .../Timeseries/Stories.tsx | 35 +++++- .../Timeseries/negativeNumData.ts | 111 ++++++++++++++++ .../src/Timeseries/transformProps.ts | 2 + .../src/Timeseries/transformers.ts | 5 +- .../plugin-chart-echarts/src/utils/series.ts | 15 ++- .../test/utils/series.test.ts | 119 ++++++++++++++++++ 6 files changed, 284 insertions(+), 3 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-demo/storybook/stories/plugins/plugin-chart-echarts/Timeseries/negativeNumData.ts 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 3f219ed6e66a..b44ba252ea5d 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 @@ -25,6 +25,7 @@ import { TimeseriesTransformProps, } from '@superset-ui/plugin-chart-echarts'; import data from './data'; +import negativeNumData from './negativeNumData'; import { withResizableChartDemo } from '../../../../shared/components/ResizableChartDemo'; new EchartsTimeseriesChartPlugin() @@ -61,7 +62,9 @@ export const Timeseries = ({ width, height }) => { chartType="echarts-timeseries" width={width} height={height} - queriesData={[{ data: queryData }]} + queriesData={[ + { data: queryData, colnames: ['__timestamp'], coltypes: [2] }, + ]} formData={{ contributionMode: undefined, forecastEnabled, @@ -87,3 +90,33 @@ export const Timeseries = ({ width, height }) => { /> ); }; + +export const WithNegativeNumbers = ({ width, height }) => ( + +); diff --git a/superset-frontend/packages/superset-ui-demo/storybook/stories/plugins/plugin-chart-echarts/Timeseries/negativeNumData.ts b/superset-frontend/packages/superset-ui-demo/storybook/stories/plugins/plugin-chart-echarts/Timeseries/negativeNumData.ts new file mode 100644 index 000000000000..8dc0f7e9b9a2 --- /dev/null +++ b/superset-frontend/packages/superset-ui-demo/storybook/stories/plugins/plugin-chart-echarts/Timeseries/negativeNumData.ts @@ -0,0 +1,111 @@ +/* + * 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 default [ + { + __timestamp: 1619827200000, + Boston: -0.88, + NewYork: null, + Washington: -0.3, + JerseyCity: -3.05, + Denver: -8.25, + SF: -0.13, + }, + { + __timestamp: 1622505600000, + Boston: -0.81, + NewYork: null, + Washington: -0.29, + JerseyCity: -3.54, + Denver: -13.4, + SF: -0.12, + }, + { + __timestamp: 1625097600000, + Boston: 0.91, + NewYork: null, + Washington: 0.25, + JerseyCity: 7.17, + Denver: 7.69, + SF: 0.05, + }, + { + __timestamp: 1627776000000, + Boston: -1.05, + NewYork: -1.04, + Washington: -0.19, + JerseyCity: -8.99, + Denver: -7.99, + SF: -0.01, + }, + { + __timestamp: 1630454400000, + Boston: -0.92, + NewYork: -1.09, + Washington: -0.17, + JerseyCity: -8.75, + Denver: -7.55, + SF: -0.01, + }, + { + __timestamp: 1633046400000, + Boston: 0.79, + NewYork: -0.85, + Washington: 0.13, + JerseyCity: 12.59, + Denver: 3.34, + SF: -0.05, + }, + { + __timestamp: 1635724800000, + Boston: 0.72, + NewYork: 0.54, + Washington: 0.15, + JerseyCity: 11.03, + Denver: 7.24, + SF: -0.14, + }, + { + __timestamp: 1638316800000, + Boston: 0.61, + NewYork: 0.73, + Washington: 0.15, + JerseyCity: 13.45, + Denver: 5.98, + SF: -0.22, + }, + { + __timestamp: 1640995200000, + Boston: 0.51, + NewYork: 1.8, + Washington: 0.15, + JerseyCity: 12.96, + Denver: 3.22, + SF: -0.02, + }, + { + __timestamp: 1643673600000, + Boston: -0.47, + NewYork: null, + Washington: -0.18, + JerseyCity: -14.27, + Denver: -6.24, + SF: -0.04, + }, +]; 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 d675d6cfab68..39361abdd741 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -170,6 +170,8 @@ export default function transformProps( }); const showValueIndexes = extractShowValueIndexes(rawSeries, { stack, + onlyTotal, + isHorizontal, }); const seriesContexts = extractForecastSeriesContexts( Object.values(rawSeries).map(series => series.name as string), 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 fe7c22a13eee..5b5514da7463 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -241,7 +241,10 @@ export function transformSeries( if (!formatter) return numericValue; if (!stack || isSelectedLegend) return formatter(numericValue); if (!onlyTotal) { - if (numericValue >= thresholdValues[dataIndex]) { + if ( + numericValue >= + (thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER) + ) { return formatter(numericValue); } return ''; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts index 24831cee3a03..e8c6c0e8bafe 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts @@ -77,6 +77,8 @@ export function extractShowValueIndexes( series: SeriesOption[], opts: { stack: StackType; + onlyTotal?: boolean; + isHorizontal?: boolean; }, ): number[] { const showValueIndexes: number[] = []; @@ -84,9 +86,20 @@ export function extractShowValueIndexes( series.forEach((entry, seriesIndex) => { const { data = [] } = entry; (data as [any, number][]).forEach((datum, dataIndex) => { - if (datum[1] !== null) { + if (!opts.onlyTotal && datum[opts.isHorizontal ? 0 : 1] !== null) { showValueIndexes[dataIndex] = seriesIndex; } + if (opts.onlyTotal) { + if (datum[opts.isHorizontal ? 0 : 1] > 0) { + showValueIndexes[dataIndex] = seriesIndex; + } + if ( + !showValueIndexes[dataIndex] && + datum[opts.isHorizontal ? 0 : 1] !== null + ) { + showValueIndexes[dataIndex] = seriesIndex; + } + } }); }); } diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts index ae3871c821e2..3f20e0d5f8a3 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts @@ -25,6 +25,7 @@ import { getChartPadding, getLegendProps, sanitizeHtml, + extractShowValueIndexes, } from '../../src/utils/series'; import { LegendOrientation, LegendType } from '../../src/types'; import { defaultLegendPadding } from '../../src/defaults'; @@ -206,6 +207,124 @@ describe('extractGroupbyLabel', () => { }); }); +describe('extractShowValueIndexes', () => { + it('should return the latest index for stack', () => { + expect( + extractShowValueIndexes( + [ + { + id: 'abc', + name: 'abc', + data: [ + ['2000-01-01', null], + ['2000-02-01', 0], + ['2000-03-01', 1], + ['2000-04-01', 0], + ['2000-05-01', null], + ['2000-06-01', 0], + ['2000-07-01', 2], + ['2000-08-01', 3], + ['2000-09-01', null], + ['2000-10-01', null], + ], + }, + { + id: 'def', + name: 'def', + data: [ + ['2000-01-01', null], + ['2000-02-01', 0], + ['2000-03-01', null], + ['2000-04-01', 0], + ['2000-05-01', null], + ['2000-06-01', 0], + ['2000-07-01', 2], + ['2000-08-01', 3], + ['2000-09-01', null], + ['2000-10-01', 0], + ], + }, + { + id: 'def', + name: 'def', + data: [ + ['2000-01-01', null], + ['2000-02-01', null], + ['2000-03-01', null], + ['2000-04-01', null], + ['2000-05-01', null], + ['2000-06-01', 3], + ['2000-07-01', null], + ['2000-08-01', null], + ['2000-09-01', null], + ['2000-10-01', null], + ], + }, + ], + { stack: true, onlyTotal: false, isHorizontal: false }, + ), + ).toEqual([undefined, 1, 0, 1, undefined, 2, 1, 1, undefined, 1]); + }); + + it('should handle the negative numbers for total only', () => { + expect( + extractShowValueIndexes( + [ + { + id: 'abc', + name: 'abc', + data: [ + ['2000-01-01', null], + ['2000-02-01', 0], + ['2000-03-01', -1], + ['2000-04-01', 0], + ['2000-05-01', null], + ['2000-06-01', 0], + ['2000-07-01', -2], + ['2000-08-01', -3], + ['2000-09-01', null], + ['2000-10-01', null], + ], + }, + { + id: 'def', + name: 'def', + data: [ + ['2000-01-01', null], + ['2000-02-01', 0], + ['2000-03-01', null], + ['2000-04-01', 0], + ['2000-05-01', null], + ['2000-06-01', 0], + ['2000-07-01', 2], + ['2000-08-01', -3], + ['2000-09-01', null], + ['2000-10-01', 0], + ], + }, + { + id: 'def', + name: 'def', + data: [ + ['2000-01-01', null], + ['2000-02-01', 0], + ['2000-03-01', null], + ['2000-04-01', 1], + ['2000-05-01', null], + ['2000-06-01', 0], + ['2000-07-01', -2], + ['2000-08-01', 3], + ['2000-09-01', null], + ['2000-10-01', 0], + ], + }, + ], + { stack: true, onlyTotal: true, isHorizontal: false }, + ), + ).toEqual([undefined, 1, 0, 2, undefined, 1, 1, 2, undefined, 1]); + }); +}); + describe('formatSeriesName', () => { const numberFormatter = getNumberFormatter(); const timeFormatter = getTimeFormatter();