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 dd054d42b50..01674bf29d5 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -31,6 +31,7 @@ import { ValueFormatter, } from '@superset-ui/core'; import { SupersetTheme, isThemeDark } from '@apache-superset/core/ui'; +import { getContrastingColor } from '@superset-ui/core'; import type { CallbackDataParams, DefaultStatesMixin, @@ -142,28 +143,39 @@ export const getBaselineSeriesForStream = ( }; }; -export function transformNegativeLabelsPosition( +export function optimizeBarLabelPlacement( series: SeriesOption, isHorizontal: boolean, ): TimeseriesDataRecord[] { /* - * Adjusts label position for negative values in bar series + * Adjusts label position for all values in bar series + * Positions labels inside bars at appropriate edges to avoid axis overlap * @param series - Array of series options * @param isHorizontal - Whether chart is horizontal - * @returns data with adjusted label positions for negative values + * @returns data with adjusted label positions for all values */ const transformValue = (value: any) => { const [xValue, yValue] = Array.isArray(value) ? value : [null, null]; const axisValue = isHorizontal ? xValue : yValue; - return axisValue < 0 - ? { - value, - label: { - position: 'outside', - }, - } - : value; + if (axisValue === null || axisValue === undefined) { + return value; + } + + // Use inside positioning for all bar charts to avoid axis overlap + const labelPosition = + axisValue < 0 + ? isHorizontal + ? 'insideLeft' + : 'insideBottom' + : isHorizontal + ? 'insideRight' + : 'insideTop'; + + return { + value, + label: { position: labelPosition }, + }; }; return (series.data as TimeseriesDataRecord[]).map(transformValue); @@ -337,8 +349,10 @@ export function transformSeries( return { ...series, - ...(Array.isArray(data) && seriesType === 'bar' && !stack - ? { data: transformNegativeLabelsPosition(series, isHorizontal) } + ...(Array.isArray(data) && seriesType === 'bar' + ? { + data: optimizeBarLabelPlacement(series, isHorizontal), + } : null), connectNulls, queryIndex, @@ -371,8 +385,11 @@ export function transformSeries( symbolSize: markerSize, label: { show: !!showValue, - position: isHorizontal ? 'right' : 'top', - color: theme?.colorText, + position: stack ? 'inside' : isHorizontal ? 'right' : 'top', + color: + stack || seriesType === 'bar' + ? getContrastingColor(String(itemStyle.color)) + : theme?.colorText, textBorderWidth: 0, formatter: (params: any) => { // don't show confidence band value labels, as they're already visible on the tooltip diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts index ae7a7423f72..68f48196df2 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts @@ -22,20 +22,24 @@ import { supersetTheme } from '@apache-superset/core/ui'; import type { SeriesOption } from 'echarts'; import { EchartsTimeseriesSeriesType } from '../../src'; import { TIMESERIES_CONSTANTS } from '../../src/constants'; -import { LegendOrientation } from '../../src/types'; +import { + LegendOrientation, + EchartsTimeseriesChartProps, +} from '../../src/types'; import { transformSeries, - transformNegativeLabelsPosition, + optimizeBarLabelPlacement, getPadding, } from '../../src/Timeseries/transformers'; import transformProps from '../../src/Timeseries/transformProps'; -import { EchartsTimeseriesChartProps } from '../../src/types'; import * as seriesUtils from '../../src/utils/series'; -// Mock the colorScale function -const mockColorScale = jest.fn( - (key: string, sliceId?: number) => `color-for-${key}-${sliceId}`, -) as unknown as CategoricalColorScale; +// Mock the colorScale function to return different colors based on key +const mockColorScale = jest.fn((key: string) => { + if (key === 'test-key') return '#1f77b4'; // blue + if (key === 'series-key') return '#ff7f0e'; // orange + return '#2ca02c'; // green for any other key +}) as unknown as CategoricalColorScale; describe('transformSeries', () => { const series = { name: 'test-series' }; @@ -49,7 +53,8 @@ describe('transformSeries', () => { const result = transformSeries(series, mockColorScale, 'test-key', opts); - expect((result as any)?.itemStyle.color).toBe('color-for-test-key-1'); + expect(mockColorScale).toHaveBeenCalledWith('test-key', 1); + expect((result as any)?.itemStyle.color).toBe('#1f77b4'); }); test('should use seriesKey if timeShiftColor is not enabled', () => { @@ -61,7 +66,8 @@ describe('transformSeries', () => { const result = transformSeries(series, mockColorScale, 'test-key', opts); - expect((result as any)?.itemStyle.color).toBe('color-for-series-key-2'); + expect(mockColorScale).toHaveBeenCalledWith('series-key', 2); + expect((result as any)?.itemStyle.color).toBe('#ff7f0e'); }); test('should apply border styles for bar series with connectNulls', () => { @@ -123,8 +129,8 @@ describe('transformSeries', () => { }); }); -describe('transformNegativeLabelsPosition', () => { - test('label position bottom of negative value no Horizontal', () => { +describe('optimizeBarLabelPlacement', () => { + test('label position for non-stacked vertical charts', () => { const isHorizontal = false; const series: SeriesOption = { data: [ @@ -137,15 +143,12 @@ describe('transformNegativeLabelsPosition', () => { type: EchartsTimeseriesSeriesType.Bar, stack: undefined, }; - const result = - Array.isArray(series.data) && series.type === 'bar' && !series.stack - ? transformNegativeLabelsPosition(series, isHorizontal) - : series.data; - expect((result as any)[0].label).toBe(undefined); - expect((result as any)[1].label).toBe(undefined); - expect((result as any)[2].label.position).toBe('outside'); - expect((result as any)[3].label.position).toBe('outside'); - expect((result as any)[4].label).toBe(undefined); + const result = optimizeBarLabelPlacement(series, isHorizontal); + expect((result as any)[0].label.position).toBe('insideTop'); + expect((result as any)[1].label.position).toBe('insideTop'); + expect((result as any)[2].label.position).toBe('insideBottom'); + expect((result as any)[3].label.position).toBe('insideBottom'); + expect((result as any)[4].label.position).toBe('insideTop'); }); test('label position left of negative value is Horizontal', () => { @@ -162,15 +165,12 @@ describe('transformNegativeLabelsPosition', () => { stack: undefined, }; - const result = - Array.isArray(series.data) && series.type === 'bar' && !series.stack - ? transformNegativeLabelsPosition(series, isHorizontal) - : series.data; - expect((result as any)[0].label).toBe(undefined); - expect((result as any)[1].label.position).toBe('outside'); - expect((result as any)[2].label).toBe(undefined); - expect((result as any)[3].label.position).toBe('outside'); - expect((result as any)[4].label.position).toBe('outside'); + const result = optimizeBarLabelPlacement(series, isHorizontal); + expect((result as any)[0].label.position).toBe('insideRight'); + expect((result as any)[1].label.position).toBe('insideLeft'); + expect((result as any)[2].label.position).toBe('insideRight'); + expect((result as any)[3].label.position).toBe('insideLeft'); + expect((result as any)[4].label.position).toBe('insideLeft'); }); test('label position to line type', () => { @@ -192,7 +192,7 @@ describe('transformNegativeLabelsPosition', () => { !series.stack && series.type !== 'line' && series.type === 'bar' - ? transformNegativeLabelsPosition(series, isHorizontal) + ? optimizeBarLabelPlacement(series, isHorizontal) : series.data; expect((result as any)[0].label).toBe(undefined); expect((result as any)[1].label).toBe(undefined); @@ -215,15 +215,34 @@ describe('transformNegativeLabelsPosition', () => { stack: 'obs', }; - const result = - Array.isArray(series.data) && series.type === 'bar' && !series.stack - ? transformNegativeLabelsPosition(series, isHorizontal) - : series.data; - expect((result as any)[0].label).toBe(undefined); - expect((result as any)[1].label).toBe(undefined); - expect((result as any)[2].label).toBe(undefined); - expect((result as any)[3].label).toBe(undefined); - expect((result as any)[4].label).toBe(undefined); + const result = optimizeBarLabelPlacement(series, isHorizontal); + expect((result as any)[0].label.position).toBe('insideTop'); + expect((result as any)[1].label.position).toBe('insideTop'); + expect((result as any)[2].label.position).toBe('insideBottom'); + expect((result as any)[3].label.position).toBe('insideBottom'); + expect((result as any)[4].label.position).toBe('insideTop'); + }); + + test('label position for horizontal stacked charts', () => { + const isHorizontal = true; + const series: SeriesOption = { + data: [ + [1, 2020], + [-3, 2021], + [2, 2022], + [-4, 2023], + [-6, 2024], + ], + type: EchartsTimeseriesSeriesType.Bar, + stack: 'obs', + }; + + const result = optimizeBarLabelPlacement(series, isHorizontal); + expect((result as any)[0].label.position).toBe('insideRight'); + expect((result as any)[1].label.position).toBe('insideLeft'); + expect((result as any)[2].label.position).toBe('insideRight'); + expect((result as any)[3].label.position).toBe('insideLeft'); + expect((result as any)[4].label.position).toBe('insideLeft'); }); });