bito-code-review[bot] commented on code in PR #42151:
URL: https://github.com/apache/superset/pull/42151#discussion_r3607223547
##########
superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts:
##########
@@ -673,217 +633,116 @@ const getAngleChartProps = (
}) as EchartsPieChartProps;
};
-test('sets center to 70% for half-donut', () => {
- const props = getAngleChartProps(true, 180);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
- expect(series[0].center).toEqual(['50%', '70%']);
-});
+const getSeries = (props: EchartsPieChartProps) =>
+ transformProps(props).echartOptions.series as PieSeriesOption[];
-test('keeps center at 50% for full donut', () => {
- const props = getAngleChartProps(true, 360);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
- expect(series[0].center).toEqual(['50%', '50%']);
+test('keeps ECharts default layout for a full donut', () => {
+ const series = getSeries(getAngleChartProps(true, 360, 90));
+ expect(series[0].center).toEqual([400, 300]);
+ expect(series[0].radius).toEqual(['30%', '70%']);
});
-test('calculates endAngle for a quarter donut', () => {
- const props = getAngleChartProps(true, 90);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
- expect(series[0].endAngle).toBe(90);
+test('recenters and scales up a top half-donut', () => {
+ // Bounding box is 2 wide x 1 tall, so an 800x600 canvas fits a radius
+ // basis of min(800/2, 600/1) = 400 instead of 300: scale 4/3.
+ const series = getSeries(getAngleChartProps(true, 180, 180));
+ expect(series[0].center).toEqual([400, 440]);
+ expect(series[0].radius).toEqual(['40%', '93.33%']);
});
-test('sets center to 30% for bottom half-donut (startAngle=0)', () => {
- const props = getAngleChartProps(true, 180, 0);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
- expect(series[0].center).toEqual(['50%', '30%']);
+test('recenters a bottom half-donut upwards', () => {
+ const series = getSeries(getAngleChartProps(true, 180, 0));
+ expect(series[0].center).toEqual([400, 160]);
});
-test('sets center to 30% for bottom half-donut (startAngle=360)', () => {
- const props = getAngleChartProps(true, 180, 360);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
- expect(series[0].center).toEqual(['50%', '30%']);
+test('recenters a right half-donut leftwards without scaling', () => {
+ // A lateral half is 1 wide x 2 tall; height binds at the full-circle
+ // basis, so the radius stays put and only the center shifts.
+ const series = getSeries(getAngleChartProps(true, 180, 90));
+ expect(series[0].center).toEqual([295, 300]);
+ expect(series[0].radius).toEqual(['30%', '70%']);
});
-test('shifts center left for right half-donut (startAngle=90)', () => {
- const props = getAngleChartProps(true, 180, 90);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
- expect(series[0].center).toEqual(['40%', '50%']);
+test('recenters a left half-donut rightwards', () => {
+ const series = getSeries(getAngleChartProps(true, 180, 270));
+ expect(series[0].center).toEqual([505, 300]);
});
-test('shifts center right for left half-donut (startAngle=270)', () => {
- const props = getAngleChartProps(true, 180, 270);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
- expect(series[0].center).toEqual(['60%', '50%']);
+test('recenters non-cardinal start angles too', () => {
+ const series = getSeries(getAngleChartProps(true, 180, 170));
+ expect(series[0].center).not.toEqual([400, 300]);
});
-test('keeps center at 50% for non-cardinal start angle even when sweep ≤ 180',
() => {
- const props = getAngleChartProps(true, 180, 45);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
- expect(series[0].center).toEqual(['50%', '50%']);
+test('scales a quarter donut to the fit cap', () => {
+ const series = getSeries(getAngleChartProps(true, 90, 180));
+ expect(series[0].center).toEqual([610, 510]);
+ expect(series[0].radius).toEqual(['60%', '140%']);
});
-test('allows endAngle to go negative for right half-donut', () => {
- const props = getAngleChartProps(true, 180, 90);
- const transformed = transformProps(props);
- const series = transformed.echartOptions.series as PieSeriesOption[];
+test('passes startAngle through and derives endAngle from the sweep', () => {
+ const series = getSeries(getAngleChartProps(true, 180, 90));
expect(series[0].startAngle).toBe(90);
expect(series[0].endAngle).toBe(-90);
});
-test.each([
- [180, 180, 'top'],
- [180, 90, 'top'],
- [180, 45, 'top'],
- [0, 180, 'bottom'],
- [360, 180, 'bottom'],
- [360, 90, 'bottom'],
- [90, 180, 'right'],
- [90, 90, 'right'],
- [270, 180, 'left'],
- [270, 90, 'left'],
- [45, 180, 'none'],
- [170, 180, 'none'],
- [180, 360, 'none'],
- [180, 181, 'none'],
- [0, 360, 'none'],
-])('startAngle=%i, sweptAngle=%i → %s', (start, swept, expected) => {
- expect(getHalfDonut(start, swept)).toBe(expected);
-});
-
-const baseProps = {
- donut: true,
- width: 800,
- height: 600,
- startAngle: 180,
- sweptAngle: 360,
-};
-
-test('returns "middle" for donut without padding and not half', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- chartPadding: { top: 0, bottom: 0, left: 0, right: 0 },
- });
- expect(result.top).toBe('middle');
-});
-
-test('returns "0" for non-donut without padding and not half', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- donut: false,
- chartPadding: { top: 0, bottom: 0, left: 0, right: 0 },
- });
- expect(result.top).toBe('0');
-});
-
-test('adjusts top for donut with bottom padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- chartPadding: { top: 0, bottom: 60, left: 0, right: 0 },
- });
- expect(result.top).toBe('45%');
-});
-
-test('returns "0" for non-donut with bottom padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- donut: false,
- chartPadding: { top: 0, bottom: 60, left: 0, right: 0 },
- });
- expect(result.top).toBe('0');
-});
-
-test('adjusts top for donut with top padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- chartPadding: { top: 60, bottom: 0, left: 0, right: 0 },
- });
- expect(result.top).toBe('55%');
-});
-
-test('adjusts top for non-donut with top padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- donut: false,
- chartPadding: { top: 60, bottom: 0, left: 0, right: 0 },
- });
- expect(result.top).toBe('10%');
-});
-
-test('positions total at 68.5% for top half-donut without padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- sweptAngle: 180,
- chartPadding: { top: 0, bottom: 0, left: 0, right: 0 },
- });
- expect(result.top).toBe('68.5%');
-});
-
-test('adjusts total position from 68.5% base for top half-donut with top
padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- sweptAngle: 180,
- chartPadding: { top: 60, bottom: 0, left: 0, right: 0 },
- });
- expect(result.top).toBe('73.5%');
-});
-
-test('returns "center" when no left/right padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- chartPadding: { top: 0, bottom: 0, left: 0, right: 0 },
- });
- expect(result.left).toBe('center');
-});
-
-test('adjusts left for left padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- chartPadding: { top: 0, bottom: 0, left: 80, right: 0 },
- });
- expect(result.left).toBe('52.5%');
+test('anchors the total on the pie origin for a top half-donut', () => {
+ const transformed = transformProps(getAngleChartProps(true, 180, 180));
+ const graphic = transformed.echartOptions.graphic as {
+ x: number;
+ y: number;
+ };
+ expect(graphic.x).toBe(400);
+ expect(graphic.y).toBe(440);
});
-test('adjusts left for right padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- chartPadding: { top: 0, bottom: 0, left: 0, right: 80 },
- });
- expect(result.left).toBe('42.5%');
+test.each([
+ ['full circle', 90, 360, 0.3, { minX: -1, maxX: 1, minY: -1, maxY: 1 }],
+ ['top half', 180, 180, 0, { minX: -1, maxX: 1, minY: 0, maxY: 1 }],
+ ['bottom half', 0, 180, 0, { minX: -1, maxX: 1, minY: -1, maxY: 0 }],
+ ['right half', 90, 180, 0, { minX: 0, maxX: 1, minY: -1, maxY: 1 }],
+ ['left half', 270, 180, 0, { minX: -1, maxX: 0, minY: -1, maxY: 1 }],
+ ['top-left quarter', 180, 90, 0.5, { minX: -1, maxX: 0, minY: 0, maxY: 1 }],
+])('getArcBoundingBox: %s', (_label, start, sweep, inner, expected) => {
+ const box = getArcBoundingBox(start, sweep, inner);
+ expect(box.minX).toBeCloseTo(expected.minX, 10);
+ expect(box.maxX).toBeCloseTo(expected.maxX, 10);
+ expect(box.minY).toBeCloseTo(expected.minY, 10);
+ expect(box.maxY).toBeCloseTo(expected.maxY, 10);
});
-test('prioritizes right padding over left padding', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- chartPadding: { top: 0, bottom: 0, left: 80, right: 80 },
- });
- expect(result.left).toBe('42.5%');
+test('getArcBoundingBox includes inner arc endpoints for narrow donuts', () =>
{
+ // A 20-degree sliver straddling 12 o'clock: the lowest point of the
+ // annular sector is an inner endpoint, not an outer one.
+ const box = getArcBoundingBox(100, 20, 0.5);
+ expect(box.minY).toBeCloseTo(0.5 * Math.sin((80 * Math.PI) / 180), 10);
+ expect(box.maxY).toBeCloseTo(1, 10);
});
-test('positions total inside the shifted center for left half-donut', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- startAngle: 270,
- sweptAngle: 180,
- chartPadding: { top: 0, bottom: 0, left: 0, right: 0 },
- });
- expect(result.left).toBe('55%');
- expect(result.top).toBe('50%');
+test('getPieLayout centers the bounding box within legend padding', () => {
+ const layout = getPieLayout({
+ width: 800,
+ height: 600,
+ padding: { top: 0, bottom: 0, left: 0, right: 200 },
+ startAngle: 90,
+ sweptAngle: 360,
+ donut: true,
+ innerRadius: 30,
+ outerRadius: 70,
+ });
+ // Rect is 600x600; the pie centers within it and the total anchor is
+ // reported in container coordinates.
+ expect(layout.center).toEqual([300, 300]);
+ expect(layout.totalAnchor).toEqual({ x: 300, y: 300 });
});
-test('positions total inside the shifted center for right half-donut', () => {
- const result = getTotalValuePadding({
- ...baseProps,
- startAngle: 90,
- sweptAngle: 180,
- chartPadding: { top: 0, bottom: 0, left: 0, right: 0 },
- });
- expect(result.left).toBe('35%');
- expect(result.top).toBe('50%');
+test('clamps the total anchor into the arc box for narrow arcs', () => {
+ // A 20-degree sliver's pie origin falls far below the drawn wedge; the
+ // anchor must stay within the arc's bounding box so the text is visible.
+ const transformed = transformProps(getAngleChartProps(true, 20, 100));
+ const graphic = transformed.echartOptions.graphic as {
+ x: number;
+ y: number;
+ };
+ expect(graphic.x).toBe(400);
+ expect(graphic.y).toBeCloseTo(421.37, 1);
Review Comment:
<!-- Bito Reply -->
The suggestion to consolidate the type extraction is valid. Applying this
change improves the code by reducing redundancy and ensuring consistency with
the `getGraphic` helper, as requested by the reviewer.
**superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts**
```
const getSeries = (props: EchartsPieChartProps) =>
transformProps(props).echartOptions.series as PieSeriesOption[];
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
