EnxDev commented on PR #35459: URL: https://github.com/apache/superset/pull/35459#issuecomment-4966513882
## EnxDev's Review Agent — apache/superset#35459 · HEAD 939a16a request changes — most prior feedback is addressed (tests added, `Swept angle` floor, constants + arg order cleaned up), but the one blocking item stands: half-donut re-centering still fires only when `startAngle === 180`, so the obvious "set Swept angle = 180" path renders off-center. Re-review of [my prior review](https://github.com/apache/superset/pull/35459#issuecomment-4918890990) (HEAD `8ea2986`). Fixed since: ✅ test coverage added (`isHalfDonut` boundaries, `center`, `endAngle`, padding branches); ✅ `sweptAngle` control now `min: 10`; ✅ constants collapsed to `HALF_DONUT_OFFSET = 68.5` (no more stray `69`); ✅ `isHalfDonut(startAngle, sweptAngle)` reads in natural order. The `getTotalValuePadding` rewrite re-verified against `master`: still behaviorally identical for existing (non-half) charts — `calculateTop`/`calculateLeft` preserve the old bottom-over-top and right-over-left precedence with the same formulas — and defaults (`startAngle 90 / sweptAngle 360 → endAngle −270`) reproduce ECharts' own defaults, so saved charts are unchanged. The widely-flagged "NaN when the angle field is cleared" is a non-issue: both controls declare non-null `default`s, and the control reducer re-applies `default` whenever a `NumberControl` is cleared to `undefined` (`getControlState.ts:124-128` — `if (state.default != null && value == null) value = state.default`). So `startAngle`/`sweptAngle` never reach `transformProps` as `undefined`; `endAngle` can't become `NaN`. The author's reply on that thread is correct. ### 🔴 Functional - **`transformProps.ts` · `isHalfDonut` (`sweptAngle <= 180 && startAngle === 180`)** · _Medium_ — Unchanged from the prior review, and the new test `isHalfDonut(90, 180) → false` now entrenches it. The two controls accept any angle, so the natural way to make a half donut — set `Swept angle = 180`, leave `Start angle` at its default 90 (or pick 0 / 270) — is not detected as half: `center` stays `['50%','50%']` and the Total text takes the full-donut path, so the arc hangs off-center and the total is mispositioned, with no in-UI hint that `Start angle` must also be exactly 180. Either derive the layout from geometry (re-center whenever `sweptAngle < 360`, shifting the center based on `startAngle`) or replace the two free-form controls with a supported-configuration control (e.g. a `Half donut` toggle) so only renderable combos are reachable. **regression test:** `transformProps({ startAngle: 90, sweptAngle: 180 })` should still re-center (`center[1] !== '50%'`) and place the total off the geometric middle. ### 🙌 Praise - `test/Pie/transformProps.test.ts` — the added suite asserts real values across the new branches (`isHalfDonut` boundaries, `center`, `endAngle`, donut/non-donut + legend-position padding); genuine regression guards, not smoke tests. <!-- enxdev-review-agent:939a16a --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 939a16a._ -- 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]
