feat(pie): geometric recentering and scaling for partial arcs#42151
Open
rusackas wants to merge 2 commits into
Open
feat(pie): geometric recentering and scaling for partial arcs#42151rusackas wants to merge 2 commits into
rusackas wants to merge 2 commits into
Conversation
Follow-up to #35459. Replaces the hardcoded half-donut layout table with geometry derived from the arc itself: - getArcBoundingBox computes the arc's bounding box from the start/sweep angles and inner radius (outer endpoints, inner endpoints, and every axis extreme the arc sweeps through) - getPieLayout scales the radius until the box fills the padded series rect (capped at the 2x factor a quarter arc reaches naturally) and shifts the pie origin so the box is centered, so partial arcs reclaim the space a full circle would leave empty - Recentering applies to ANY start/sweep combination, removing the previous behavior cliff at non-cardinal start angles - The Total label anchors on the pie origin via graphic text align/verticalAlign instead of tuned percentage offsets, clamped into the arc box so narrow arcs keep it on-canvas - A full circle reproduces the ECharts default layout exactly, so existing charts are unchanged - Sweep-angle control description updated; translation catalogs synced Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rusackas
requested review from
hainenber,
sadpandajoe,
sfirke and
villebro
as code owners
July 18, 2026 00:02
github-actions
Bot
added
i18n
dosubot
Bot
added
change:frontend
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #ac672d
Actionable Suggestions - 10
-
superset/translations/zh/LC_MESSAGES/messages.po - 1
- Missing Chinese translation · Line 17530-17534
-
superset/translations/sl/LC_MESSAGES/messages.po - 1
- Missing translation for updated msgid · Line 16959-16959
-
superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts - 2
- Consolidate duplicate type extraction · Line 690-695
- Consolidate duplicate type extraction · Line 742-747
-
superset/translations/ja/LC_MESSAGES/messages.po - 1
- CWE-1373: Incomplete Translation · Line 14399-14403
-
superset/translations/sr_Latn/LC_MESSAGES/messages.po - 1
- Incomplete Serbian translation · Line 19575-19579
-
superset/translations/es/LC_MESSAGES/messages.po - 1
- Missing Spanish translation · Line 16836-16836
-
superset/translations/fr/LC_MESSAGES/messages.po - 1
- Missing French translation · Line 16864-16864
-
superset/translations/it/LC_MESSAGES/messages.po - 1
- Missing Italian translation for tooltip · Line 21867-21871
-
superset/translations/ca/LC_MESSAGES/messages.po - 1
- Missing Catalan translation for Sweep angle description · Line 16853-16856
Additional Suggestions - 8
-
superset/translations/tr/LC_MESSAGES/messages.po - 1
-
Incomplete Turkish translation · Line 16623-16623Turkish translation for 'Total angle covered by the chart' (lines 16623-16626) has empty msgstr. Turkish-speaking users will see untranslated English text. Per BITO.md rule [13343], all user-facing messages must have complete translations before merging.
-
-
superset/translations/pt/LC_MESSAGES/messages.po - 1
-
Missing Portuguese translation · Line 24662-24662The `msgstr` for this entry is empty, leaving the help text untranslated for Portuguese users. Rule [12743] requires complete translation coverage for all user-facing messages before merging.
-
-
superset/translations/pt_BR/LC_MESSAGES/messages.po - 1
-
Incomplete Portuguese translation · Line 17447-17451The msgstr field for the "Total angle covered by the chart" entry is empty (line 17451). Portuguese users will see untranslated English text when configuring chart sweep angles. Rule [12665] prohibits merging unreviewed entries with empty translations. Rule [12743] requires complete translation coverage for all user-facing messages. Rule [13346] requires validation of technical term translations like "degrees" and "arcs".
-
-
superset/translations/ar/LC_MESSAGES/messages.po - 1
-
Incomplete translation needs filling · Line 17322-17326The msgstr at line 17326 remains empty after the source text was updated. Arabic users will see untranslated English text for this chart configuration tooltip. Per BITO.md rule [13343], complete translations are required before merging.
-
-
superset/translations/sk/LC_MESSAGES/messages.po - 1
-
Missing Slovak translation · Line 16156-16156The msgstr is empty but the msgid has been updated with new source text. Slovak users will see untranslated English text instead of their native language. Per BITO.md rule [12743], all user-facing messages require complete translations for every supported language before merging. Add the Slovak translation to ensure consistent user experience for Slovak-speaking users. ([CWE-164](https://cwe.mitre.org/data/definitions/164.html))
-
-
superset/translations/pl/LC_MESSAGES/messages.po - 1
-
Incomplete translation needs filling · Line 17756-17756The msgstr remains empty after the msgid was updated. Polish translation needs to be added for this tooltip text describing the sweep angle behavior. Rule 12743 requires complete translation coverage for all user-facing messages.
-
-
superset/translations/lv/LC_MESSAGES/messages.po - 1
-
Missing translation for updated msgid · Line 15792-15795The source text for 'Total angle covered' has been updated to match the implementation in `controlPanel.tsx` (lines 326-329), but the Latvian translation remains empty (`msgstr ""`). Provide a translation to complete localization for Latvian users.
-
-
superset/translations/de/LC_MESSAGES/messages.po - 1
-
Incomplete German translation · Line 15800-15803The English tooltip text was updated (partial arcs re-centering behavior), but the `msgstr` at line 15803 remains empty. Consider providing the German translation to complete this entry per translation coverage standards.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/translations/ro/LC_MESSAGES/messages.po - 1
- Translation markers not removed · Line 17017-17019
-
superset/translations/th/LC_MESSAGES/messages.po - 1
- Unreviewed machine-translation marker persists · Line 27440-27442
Review Details
-
Files reviewed - 34 · Commit Range:
45c4c97..45c4c97- superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Pie/types.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts
- superset/translations/ar/LC_MESSAGES/messages.po
- superset/translations/ca/LC_MESSAGES/messages.po
- superset/translations/cs/LC_MESSAGES/messages.po
- superset/translations/de/LC_MESSAGES/messages.po
- superset/translations/en/LC_MESSAGES/messages.po
- superset/translations/es/LC_MESSAGES/messages.po
- superset/translations/fa/LC_MESSAGES/messages.po
- superset/translations/fi/LC_MESSAGES/messages.po
- superset/translations/fr/LC_MESSAGES/messages.po
- superset/translations/it/LC_MESSAGES/messages.po
- superset/translations/ja/LC_MESSAGES/messages.po
- superset/translations/ko/LC_MESSAGES/messages.po
- superset/translations/lv/LC_MESSAGES/messages.po
- superset/translations/messages.pot
- superset/translations/mi/LC_MESSAGES/messages.po
- superset/translations/nl/LC_MESSAGES/messages.po
- superset/translations/pl/LC_MESSAGES/messages.po
- superset/translations/pt/LC_MESSAGES/messages.po
- superset/translations/pt_BR/LC_MESSAGES/messages.po
- superset/translations/ro/LC_MESSAGES/messages.po
- superset/translations/ru/LC_MESSAGES/messages.po
- superset/translations/sk/LC_MESSAGES/messages.po
- superset/translations/sl/LC_MESSAGES/messages.po
- superset/translations/sr/LC_MESSAGES/messages.po
- superset/translations/sr_Latn/LC_MESSAGES/messages.po
- superset/translations/th/LC_MESSAGES/messages.po
- superset/translations/tr/LC_MESSAGES/messages.po
- superset/translations/uk/LC_MESSAGES/messages.po
- superset/translations/zh/LC_MESSAGES/messages.po
- superset/translations/zh_TW/LC_MESSAGES/messages.po
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42151 +/- ##
=======================================
Coverage 65.06% 65.07%
=======================================
Files 2758 2758
Lines 155222 155224 +2
Branches 35570 35560 -10
=======================================
+ Hits 101002 101010 +8
+ Misses 52305 52299 -6
Partials 1915 1915
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
🎪 Showtime deployed environment on GHA for 45c4c97 • Environment: http://52.39.23.249:8080 (admin/admin) |
…tion Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
🎪 7cec2d4 🚦 building
🎪 7cec2d4 📅 2026-07-18T02-16
Contributor
github-actions
Bot
added
🎪 7cec2d4 🚦 deploying
🎪 7cec2d4 🚦 running
Contributor
|
🎪 Showtime deployed environment on GHA for 7cec2d4 • Environment: http://34.220.30.183:8080 (admin/admin) |
Contributor
Code Review Agent Run #915419Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
SBIN2010
approved these changes
Jul 18, 2026
SBIN2010
left a comment
SBIN2010
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the improvements! It turned out great.
For the sake of code cleanliness, it’s better to move the interfaces to a types file. Otherwise —
LGTM!
| right: { centerOffset: { x: -10, y: 0 }, totalBase: { left: 35, top: 50 } }, | ||
| none: { centerOffset: { x: 0, y: 0 }, totalBase: { left: 50, top: 50 } }, | ||
| }; | ||
| export interface ArcBoundingBox { |
Contributor
There was a problem hiding this comment.
It’s probably better to move this to types.
Member
Author
There was a problem hiding this comment.
I went back and forth on this, but I think it's fine where it is. ArcBoundingBox is tightly coupled to getArcBoundingBox right below it and only used internally (plus in tests). legendLayout.ts and treeBuilder.ts do the same co-location elsewhere in this plugin, so it matches the local convention. types.ts feels more like the place for the form data/props contract.
| */ | ||
| const MAX_RADIUS_SCALE = 2; | ||
|
|
||
| export interface PieLayout { |
Contributor
There was a problem hiding this comment.
And it's better to move this to types.
Member
Author
There was a problem hiding this comment.
Same reasoning as above, PieLayout is an implementation detail of getPieLayout right below it, not part of the shared props/form-data contract in types.ts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Follow-up to #35459, which added
Start angle/Sweep anglecontrols to the Pie chart with a hardcoded layout table that only recentered half-donuts at exact cardinal start angles (0°/90°/180°/270°), and left partial arcs at full-circle sizing (so a half donut wasted roughly half the canvas).This PR replaces that table with layout derived from the arc's actual geometry:
getArcBoundingBoxcomputes the arc's bounding box from the start/sweep angles and inner radius (outer endpoints, inner endpoints, and every axis extreme the arc sweeps through).getPieLayoutscales the radius until that box fills the padded series rect, capped at the 2× factor a quarter arc reaches naturally, and shifts the pie origin so the box is centered. Partial arcs reclaim the space a full circle would leave empty.align/verticalAligncentering instead of empirically tuned percentage offsets, and is clamped into the arc's box so narrow arcs keep it on-canvas. With side legends the label is centered exactly in the padded rect rather than approximated with fudge factors.The magic-constant table,
getHalfDonut, andgetTotalValuePaddingare deleted; the sweep-angle control description is updated accordingly and translation catalogs are synced for the reworded string.Verified visually across a 12-configuration matrix (full, all four halves, quarter, non-cardinal 170°, diagonal 45°, 20° sliver, 270° arc, legend padding, non-donut half) rendered with ECharts.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: a half donut with
Start angle170° rendered off-center at full-circle size; only exact cardinal angles recentered, without scaling.After: every partial arc is centered and scaled to use the available space.
TESTING INSTRUCTIONS
Sweep angleto 180°. The chart should render centered and noticeably larger than before.Start anglevalues that are not multiples of 90° (e.g. 170°): the chart stays centered instead of jumping.Sweep angleto 90° for a quarter donut: it scales up to fill the canvas.Show Totalwith and without legends on each side; the total stays centered on the donut hole (or the top of the chart for non-donut pies).master.ADDITIONAL INFORMATION
🤖 Generated with Claude Code