bito-code-review[bot] commented on code in PR #42151: URL: https://github.com/apache/superset/pull/42151#discussion_r3607071731
########## superset/translations/sl/LC_MESSAGES/messages.po: ########## @@ -16954,9 +16954,8 @@ msgstr "Skupaj (%(aggregatorName)s)" msgid "" "Total angle covered by the chart, in degrees. 360° draws a full circle " -"and 180° draws a half donut. When the sweep is 180° or less and the start" -" angle is a multiple of 90°, the chart is automatically re-centered to " -"make use of the empty space." +"and 180° draws a half donut. Partial arcs are automatically re-centered " +"and scaled to make use of the available space." msgstr "" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing translation for updated msgid</b></div> <div id="fix"> The 'Total angle covered by the chart' msgid has been updated but `msgstr` remains empty. This means Slovenian users will see English help text instead of a translated string. Per BITO.md [13343], all user-facing messages must have complete translations for every supported language — empty msgstr fields degrade user experience for non-English speakers. Either provide the translation or add `#, fuzzy` to mark it for review. </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Consolidate duplicate type extraction</b></div> <div id="fix"> The inline `graphic` type definition here (lines 690-693) duplicates the `getGraphic` helper (line 353) added in the same diff. Both extract `transformed.echartOptions.graphic` with identical shape. Consolidating eliminates divergence risk if the graphic structure evolves. </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/translations/it/LC_MESSAGES/messages.po: ########## @@ -21866,9 +21866,8 @@ msgstr "Totale (%(aggregatorName)s)" msgid "" "Total angle covered by the chart, in degrees. 360° draws a full circle " -"and 180° draws a half donut. When the sweep is 180° or less and the start" -" angle is a multiple of 90°, the chart is automatically re-centered to " -"make use of the empty space." +"and 180° draws a half donut. Partial arcs are automatically re-centered " +"and scaled to make use of the available space." msgstr "" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing Italian translation for tooltip</b></div> <div id="fix"> Line 21871: The `msgstr` field is empty for the 'Total angle covered' tooltip entry. Italian users will see the English text instead of their native language. This file contains 687 entries with empty msgstr fields — this entry is one of them. Rule [12743] requires complete translation coverage for all user-facing messages. </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/translations/sr_Latn/LC_MESSAGES/messages.po: ########## @@ -19574,9 +19574,8 @@ msgstr "Ukupno (%(aggregatorName)s)" msgid "" "Total angle covered by the chart, in degrees. 360° draws a full circle " -"and 180° draws a half donut. When the sweep is 180° or less and the start" -" angle is a multiple of 90°, the chart is automatically re-centered to " -"make use of the empty space." +"and 180° draws a half donut. Partial arcs are automatically re-centered " +"and scaled to make use of the available space." msgstr "" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete Serbian translation</b></div> <div id="fix"> Please update the msgstr for the "Total angle covered by the chart" entry to: "Ukupan ugao koji grafikon pokriva, u stepenima. 360° crta puni krug, a 180° crta polukruženu tortu. Delimitčni lukovi se automatski centriraju i skaliraju da iskoriste dostupan prostor." </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/translations/ja/LC_MESSAGES/messages.po: ########## @@ -14398,9 +14398,8 @@ msgstr "合計 (%(aggregatorName)s)" msgid "" "Total angle covered by the chart, in degrees. 360° draws a full circle " -"and 180° draws a half donut. When the sweep is 180° or less and the start" -" angle is a multiple of 90°, the chart is automatically re-centered to " -"make use of the empty space." +"and 180° draws a half donut. Partial arcs are automatically re-centered " +"and scaled to make use of the available space." msgstr "" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CWE-1373: Incomplete Translation</b></div> <div id="fix"> The `msgstr` for this user-facing chart configuration description is empty. Japanese users will see untranslated English text in the UI. Add a complete Japanese translation before merging per translation coverage standards. (See also: [CWE-1373](https://cwe.mitre.org/data/definitions/1373.html)) </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/translations/es/LC_MESSAGES/messages.po: ########## @@ -16831,9 +16831,8 @@ msgstr "Total (%(aggregatorName)s)" msgid "" "Total angle covered by the chart, in degrees. 360° draws a full circle " -"and 180° draws a half donut. When the sweep is 180° or less and the start" -" angle is a multiple of 90°, the chart is automatically re-centered to " -"make use of the empty space." +"and 180° draws a half donut. Partial arcs are automatically re-centered " +"and scaled to make use of the available space." msgstr "" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing Spanish translation</b></div> <div id="fix"> The `msgstr` field at line 16836 is empty, meaning Spanish users will see the untranslated English text for this chart configuration tooltip. Per rule 12743, all user-facing messages must have complete translations. Add a Spanish translation for this tooltip text. </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/translations/zh/LC_MESSAGES/messages.po: ########## @@ -17529,9 +17529,8 @@ msgstr "总计 (%(aggregatorName)s)" msgid "" "Total angle covered by the chart, in degrees. 360° draws a full circle " -"and 180° draws a half donut. When the sweep is 180° or less and the start" -" angle is a multiple of 90°, the chart is automatically re-centered to " -"make use of the empty space." +"and 180° draws a half donut. Partial arcs are automatically re-centered " +"and scaled to make use of the available space." msgstr "" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing Chinese translation</b></div> <div id="fix"> The `msgstr` field for the sweep angle description remains empty. Chinese users will see untranslated English text instead of the localized message. Per BITO.md rule [12743], all user-facing messages must have complete translations before merging. </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/translations/fr/LC_MESSAGES/messages.po: ########## @@ -16859,9 +16859,8 @@ msgstr "Total (%(aggregatorName)s)" msgid "" "Total angle covered by the chart, in degrees. 360° draws a full circle " -"and 180° draws a half donut. When the sweep is 180° or less and the start" -" angle is a multiple of 90°, the chart is automatically re-centered to " -"make use of the empty space." +"and 180° draws a half donut. Partial arcs are automatically re-centered " +"and scaled to make use of the available space." msgstr "" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing French translation</b></div> <div id="fix"> This entry now has the updated source text but empty msgstr. French users will see untranslated English text. Add the French translation before merging. </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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: <div> <div id="suggestion"> <div id="issue"><b>Consolidate duplicate type extraction</b></div> <div id="fix"> Duplicate of the semantic duplication issue at lines 690-695. Apply the same fix to consolidate this inline type extraction with the getGraphic helper. </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/translations/ca/LC_MESSAGES/messages.po: ########## @@ -16851,9 +16851,8 @@ msgstr "Total (%(aggregatorName)s)" msgid "" "Total angle covered by the chart, in degrees. 360° draws a full circle " -"and 180° draws a half donut. When the sweep is 180° or less and the start" -" angle is a multiple of 90°, the chart is automatically re-centered to " -"make use of the empty space." +"and 180° draws a half donut. Partial arcs are automatically re-centered " +"and scaled to make use of the available space." msgstr "" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing Catalan translation for Sweep angle description</b></div> <div id="fix"> The `msgid` for the 'Sweep angle' chart configuration has been updated to match the current source implementation (`controlPanel.tsx`), but the `msgstr` remains empty. Non-Catalan speakers will see untranslated English text instead of a complete localized experience. </div> </div> <small><i>Code Review Run #ac672d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
