Copilot commented on code in PR #35459:
URL: https://github.com/apache/superset/pull/35459#discussion_r2524531172
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -95,24 +97,28 @@ function getTotalValuePadding({
top: donut ? 'middle' : '0',
left: 'center',
};
+ const getDonutBase = () => (half ? 68.5 : 50);
+ if (half) {
+ padding.top = donut
+ ? `${69 + (chartPadding.top / height / 2) * 100}%`
+ : `${(chartPadding.top / height) * 100}%`;
+ }
if (chartPadding.top) {
padding.top = donut
- ? `${50 + (chartPadding.top / height / 2) * 100}%`
+ ? `${getDonutBase() + (chartPadding.top / height / 2) * 100}%`
: `${(chartPadding.top / height) * 100}%`;
}
Review Comment:
The logic is flawed because when `half` is true and `chartPadding.top` is
also truthy, the padding.top value set in lines 101-105 will be immediately
overwritten by lines 106-110. This means the hardcoded `69` value at line 103
is never used. Consider consolidating this logic or ensuring the conditions are
mutually exclusive.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:
##########
@@ -95,24 +97,28 @@ function getTotalValuePadding({
top: donut ? 'middle' : '0',
left: 'center',
};
+ const getDonutBase = () => (half ? 68.5 : 50);
Review Comment:
The magic numbers 68.5 and 69 (used on line 103) appear to be related but
are inconsistent. Consider defining these as named constants with explanatory
comments to clarify their purpose and relationship (e.g.,
`HALF_DONUT_BASE_POSITION` and `HALF_DONUT_TOP_OFFSET`).
--
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]
