EnxDev commented on PR #35459:
URL: https://github.com/apache/superset/pull/35459#issuecomment-4918890990
## EnxDev's Review Agent โ apache/superset#35459 ยท HEAD 8ea2986
request changes โ the padding refactor is behavior-preserving, but the
half-donut re-centering only fires for one exact angle combo
(`startAngle===180`), so most configurations reachable from the two new
controls render off-center; and the new logic ships with no tests.
Feature PR: adds `Start angle` / `Swept angle` controls to the Pie chart and
derives a "half donut" layout. I verified the `getTotalValuePadding` rewrite
against `master`: for existing (non-half) charts it is behaviorally identical โ
`getChartPadding` (non-horizontal branch, the one Pie uses) only ever sets one
of top/bottom/left/right, so the reordered branches produce the same result,
and the new `safeHeight/safeWidth` guards only change the previously-NaN
zero-dimension case. Defaults `startAngle:90 / sweptAngle:360` reproduce
ECharts' own defaults (`startAngle 90`, `endAngle -270`), so saved charts are
unchanged. No blocking regression there.
### ๐ด Functional
- **`transformProps.ts:428` (`isHalfDonut`) & `:431` (`center`)** ยท _Medium_
โ Re-centering keys off `startAngle === HALF_DONUT_START_ANGLE` (exactly 180)
AND `sweptAngle <= 180`. The controls advertise any angle 0โ360. So the common
case โ user sets `Swept angle = 180` but leaves `Start angle` at its default 90
(or picks 0/270) โ is *not* recognized as half: `center` stays `['50%','50%']`
and the Total text uses the full-donut path, so the half arc hangs off-center
and the total is mispositioned. Key the layout off the actual geometry (e.g.
`sweptAngle <= 180`, deriving the center shift from `startAngle`) rather than a
hardcoded start angle โ or constrain the controls so only supported combos are
selectable. **regression test:** `transformProps` with `{startAngle:90,
sweptAngle:180}` should still re-center (`center[1] !== '50%'`), and
`{startAngle:180, sweptAngle:180}` โ `center:['50%','70%']`, `startAngle:180`,
`endAngle:0`.
### ๐ก Should-fix
- **`transformProps.ts` / `test/Pie/transformProps.test.ts`** โ New exported
logic (`isHalfDonut`, the rewritten `getTotalValuePadding`,
`startAngle`/`endAngle`/`center`) has zero test coverage; the functions were
even exported but no test was added. Superset requires a regression test per
behavioral change. Add cases for: the `isHalfDonut` threshold boundaries
(`sweptAngle` 180 vs 181; `startAngle` 180 vs 90), `endAngle = startAngle -
sweptAngle`, and the donut/non-donut + legend-position padding branches.
- **`controlPanel.tsx:123` / `:138`** โ `min:0` lets `Swept angle = 0`,
which renders an empty chart with no feedback. Consider `min:10` (matching
`step`) or a sensible floor so the control can't produce an invisible viz.
### ๐ต Nits
- `transformProps.ts:76-77` โ `HALF_DONUT_BASE_POSITION = 69` vs
`HALF_DONUT_TOP_OFFSET = 68.5` are used for the same "half donut center"
concept with a 0.5% gap that looks accidental; collapse to one constant unless
the difference is intentional (add a comment if so).
- `transformProps.ts:82` โ `isHalfDonut(sweptAngle, startAngle)` takes
`(swept, start)` โ reversed from the natural `(start, swept)` reading. Both
call sites are correct, but the order is easy to mis-call later; consider
`(startAngle, sweptAngle)`.
<!-- enxdev-review-agent:8ea2986 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 8ea2986._
--
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]
