Skip to content

Commit 0d617ff

Browse files
rusackasclaude
andcommitted
fix(scatter): normalize dot size range, fall back to min dot size, drop any types in tests
- normalize an inverted min/max dot size range so larger values always render as larger dots - fall back to the minimum configured dot size (not the hidden fixed marker size control) for points missing a size value - replace any annotations/casts in scatter control panel and transformProps tests with concrete test types Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 334ed5b commit 0d617ff

3 files changed

Lines changed: 94 additions & 24 deletions

File tree

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,12 @@ export default function transformProps(
423423
}
424424
return name === matchedLabel ? '' : name.slice(matchedLabel.length + 2);
425425
};
426-
const markerSizeRange: [number, number] = [minMarkerSize, maxMarkerSize];
426+
// Normalize the configured dot size range so an inverted min/max still
427+
// scales larger metric values to larger dots.
428+
const markerSizeRange: [number, number] =
429+
minMarkerSize <= maxMarkerSize
430+
? [minMarkerSize, maxMarkerSize]
431+
: [maxMarkerSize, minMarkerSize];
427432

428433
const showValueIndexes = extractShowValueIndexes(rawSeries, {
429434
stack,
@@ -571,9 +576,12 @@ export default function transformProps(
571576
const sizeValue = sizeIsValueMetric
572577
? value[isHorizontal ? 0 : 1]
573578
: sizeLookup!.get(value[isHorizontal ? 1 : 0]);
579+
// Points with a missing/invalid size value get the smallest
580+
// configured dot size; the fixed marker size control is hidden
581+
// when a size metric is set, so its value would be stale here.
574582
return typeof sizeValue === 'number'
575583
? getAreaScaledSymbolSize(sizeValue, extent, markerSizeRange)
576-
: markerSize;
584+
: markerSizeRange[0];
577585
};
578586
}
579587
}

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,19 @@ import controlPanel from '../../../src/Timeseries/Regular/Scatter/controlPanel';
2222

2323
const config = controlPanel;
2424

25-
const getControl = (controlName: string) => {
25+
// Narrow view of a named control entry covering the config fields these
26+
// tests assert on, so lookups stay type-safe without resorting to `any`.
27+
interface TestControl {
28+
name: string;
29+
config: {
30+
default?: unknown;
31+
options?: unknown;
32+
validators?: unknown;
33+
visibility: (props: ControlPanelsContainerProps) => boolean;
34+
};
35+
}
36+
37+
const getControl = (controlName: string): TestControl | null => {
2638
for (const section of config.controlPanelSections) {
2739
if (section?.controlSetRows) {
2840
for (const row of section.controlSetRows) {
@@ -33,7 +45,7 @@ const getControl = (controlName: string) => {
3345
'name' in control &&
3446
control.name === controlName
3547
) {
36-
return control;
48+
return control as unknown as TestControl;
3749
}
3850
}
3951
}
@@ -67,7 +79,7 @@ const mockControls = (
6779
};
6880

6981
// tests for x_axis_time_format control
70-
const timeFormatControl: any = getControl('x_axis_time_format');
82+
const timeFormatControl = getControl('x_axis_time_format')!;
7183

7284
test('scatter chart control panel should include x_axis_time_format control in the panel', () => {
7385
expect(timeFormatControl).toBeDefined();
@@ -108,7 +120,7 @@ test('x_axis_time_format control should be hidden for non-temporal data types',
108120
});
109121

110122
// tests for x_axis_number_format control
111-
const numberFormatControl: any = getControl('x_axis_number_format');
123+
const numberFormatControl = getControl('x_axis_number_format')!;
112124

113125
test('scatter chart control panel should include x_axis_number_format control in the panel', () => {
114126
expect(numberFormatControl).toBeDefined();
@@ -150,10 +162,10 @@ test('x_axis_number_format control should be hidden for non-numeric data types',
150162
});
151163

152164
// tests for orientation and dot size controls
153-
const orientationControl: any = getControl('orientation');
154-
const sizeControl: any = getControl('size');
155-
const minMarkerSizeControl: any = getControl('minMarkerSize');
156-
const maxMarkerSizeControl: any = getControl('maxMarkerSize');
165+
const orientationControl = getControl('orientation')!;
166+
const sizeControl = getControl('size')!;
167+
const minMarkerSizeControl = getControl('minMarkerSize')!;
168+
const maxMarkerSizeControl = getControl('maxMarkerSize')!;
157169

158170
test('scatter chart control panel should include an orientation control defaulting to vertical', () => {
159171
expect(orientationControl).toBeDefined();
@@ -196,7 +208,7 @@ test('dot size range controls should only be visible when a size metric is set',
196208
});
197209

198210
test('fixed marker size control should hide when a size metric is set', () => {
199-
const markerSizeControl: any = getControl('markerSize');
211+
const markerSizeControl = getControl('markerSize')!;
200212
expect(markerSizeControl.config.visibility(mockSizeControls(null))).toBe(
201213
true,
202214
);

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts

Lines changed: 63 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,22 @@ import {
3232
} from '@superset-ui/chart-controls';
3333
import { supersetTheme } from '@apache-superset/core/theme';
3434

35+
type AxisLabelFormatter = ((value: number | string) => string) & {
36+
id?: string;
37+
};
38+
39+
interface TestAxis {
40+
type?: string;
41+
axisLabel: { formatter: AxisLabelFormatter };
42+
}
43+
44+
interface TestScatterSeries {
45+
type?: string;
46+
name?: string;
47+
data: (string | number | null)[][];
48+
symbolSize: (value: (string | number | null)[]) => number;
49+
}
50+
3551
describe('Scatter Chart X-axis Time Formatting', () => {
3652
const baseFormData: EchartsTimeseriesFormData = {
3753
...DEFAULT_FORM_DATA,
@@ -74,7 +90,7 @@ describe('Scatter Chart X-axis Time Formatting', () => {
7490
);
7591

7692
expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel');
77-
const xAxis = transformedProps.echartOptions.xAxis as any;
93+
const xAxis = transformedProps.echartOptions.xAxis as TestAxis;
7894
expect(xAxis.axisLabel).toHaveProperty('formatter');
7995
expect(typeof xAxis.axisLabel.formatter).toBe('function');
8096
});
@@ -94,7 +110,7 @@ describe('Scatter Chart X-axis Time Formatting', () => {
94110
chartProps as EchartsTimeseriesChartProps,
95111
);
96112

97-
const xAxis = transformedProps.echartOptions.xAxis as any;
113+
const xAxis = transformedProps.echartOptions.xAxis as TestAxis;
98114
expect(xAxis.axisLabel).toHaveProperty('formatter');
99115
expect(typeof xAxis.axisLabel.formatter).toBe('function');
100116
if (format !== SMART_DATE_ID) {
@@ -146,7 +162,7 @@ describe('Scatter Chart X-axis Number Formatting', () => {
146162
);
147163

148164
expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel');
149-
const xAxis = transformedProps.echartOptions.xAxis as any;
165+
const xAxis = transformedProps.echartOptions.xAxis as TestAxis;
150166
expect(xAxis.axisLabel).toHaveProperty('formatter');
151167
expect(typeof xAxis.axisLabel.formatter).toBe('function');
152168
expect(xAxis.axisLabel.formatter.id).toBe('SMART_NUMBER');
@@ -168,7 +184,7 @@ describe('Scatter Chart X-axis Number Formatting', () => {
168184
);
169185

170186
expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel');
171-
const xAxis = transformedProps.echartOptions.xAxis as any;
187+
const xAxis = transformedProps.echartOptions.xAxis as TestAxis;
172188
expect(xAxis.axisLabel).toHaveProperty('formatter');
173189
expect(typeof xAxis.axisLabel.formatter).toBe('function');
174190
expect(xAxis.axisLabel.formatter.id).toBe(format);
@@ -216,7 +232,9 @@ describe('Scatter Chart Orientation and Dot Size Metric', () => {
216232
};
217233

218234
const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
219-
(props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
235+
(props.echartOptions.series as TestScatterSeries[]).filter(
236+
s => s.type === 'scatter',
237+
);
220238

221239
const singleMetricData = [
222240
{
@@ -240,7 +258,10 @@ describe('Scatter Chart Orientation and Dot Size Metric', () => {
240258
const transformedProps = transformProps(
241259
chartProps as EchartsTimeseriesChartProps,
242260
);
243-
const { xAxis, yAxis } = transformedProps.echartOptions as any;
261+
const { xAxis, yAxis } = transformedProps.echartOptions as {
262+
xAxis: TestAxis;
263+
yAxis: TestAxis;
264+
};
244265
expect(yAxis.type).toBe('category');
245266
expect(xAxis.type).toBe('value');
246267

@@ -260,7 +281,10 @@ describe('Scatter Chart Orientation and Dot Size Metric', () => {
260281
const transformedProps = transformProps(
261282
chartProps as EchartsTimeseriesChartProps,
262283
);
263-
const { xAxis, yAxis } = transformedProps.echartOptions as any;
284+
const { xAxis, yAxis } = transformedProps.echartOptions as {
285+
xAxis: TestAxis;
286+
yAxis: TestAxis;
287+
};
264288
expect(xAxis.type).toBe('category');
265289
expect(yAxis.type).toBe('value');
266290

@@ -361,12 +385,12 @@ describe('Scatter Chart Orientation and Dot Size Metric', () => {
361385
chartProps as EchartsTimeseriesChartProps,
362386
);
363387
const series = getScatterSeries(transformedProps);
364-
expect(series.map((s: any) => s.name).sort()).toEqual([
388+
expect(series.map(s => s.name).sort()).toEqual([
365389
'sum_val, g1',
366390
'sum_val, g2',
367391
]);
368-
const g1 = series.find((s: any) => s.name === 'sum_val, g1');
369-
const g2 = series.find((s: any) => s.name === 'sum_val, g2');
392+
const g1 = series.find(s => s.name === 'sum_val, g1')!;
393+
const g2 = series.find(s => s.name === 'sum_val, g2')!;
370394
// the size extent is global: g1's point holds the minimum (10), g2's the
371395
// maximum (40)
372396
expect(g1.symbolSize(['A', 1])).toBe(5);
@@ -396,7 +420,7 @@ describe('Scatter Chart Orientation and Dot Size Metric', () => {
396420
expect(series.symbolSize([3, 'C'])).toBe(30);
397421
});
398422

399-
test('points without a size value fall back to the fixed marker size', () => {
423+
test('points without a size value fall back to the minimum dot size', () => {
400424
const chartProps = new ChartProps({
401425
...baseChartPropsConfig,
402426
queriesData: [
@@ -409,14 +433,40 @@ describe('Scatter Chart Orientation and Dot Size Metric', () => {
409433
],
410434
},
411435
],
412-
formData: { ...baseFormData, size: 'size_metric', markerSize: 7 },
436+
formData: {
437+
...baseFormData,
438+
size: 'size_metric',
439+
minMarkerSize: 9,
440+
maxMarkerSize: 30,
441+
},
442+
});
443+
444+
const transformedProps = transformProps(
445+
chartProps as EchartsTimeseriesChartProps,
446+
);
447+
const [series] = getScatterSeries(transformedProps);
448+
expect(series.symbolSize(['B', 2])).toBe(9);
449+
});
450+
451+
test('an inverted min/max dot size range is normalized', () => {
452+
const chartProps = new ChartProps({
453+
...baseChartPropsConfig,
454+
queriesData: categoricalData,
455+
formData: {
456+
...baseFormData,
457+
size: 'size_metric',
458+
minMarkerSize: 30,
459+
maxMarkerSize: 5,
460+
},
413461
});
414462

415463
const transformedProps = transformProps(
416464
chartProps as EchartsTimeseriesChartProps,
417465
);
418466
const [series] = getScatterSeries(transformedProps);
419-
expect(series.symbolSize(['B', 2])).toBe(7);
467+
// larger metric values still render as larger dots
468+
expect(series.symbolSize(['A', 1])).toBe(5);
469+
expect(series.symbolSize(['C', 3])).toBe(30);
420470
});
421471

422472
test('size metric equal to the value metric sizes dots by their own value', () => {

0 commit comments

Comments
 (0)