korbit-ai[bot] commented on code in PR #35459:
URL: https://github.com/apache/superset/pull/35459#discussion_r2400063999
##########
superset-frontend/plugins/plugin-chart-echarts/src/Pie/types.ts:
##########
@@ -35,6 +35,7 @@ export type EchartsPieFormData = QueryFormData &
donut: boolean;
defaultValue?: string[] | null;
groupby: QueryFormColumn[];
+ half: boolean;
Review Comment:
### Missing validation for half-donut compatibility <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The half property is defined as a required boolean field in the
EchartsPieFormData type, but there's no validation or constraint to ensure it
works correctly with the donut property.
###### Why this matters
This could lead to invalid configurations where half mode is enabled on a
regular pie chart (non-donut), which may not render correctly or cause
unexpected behavior in the visualization logic.
###### Suggested change ∙ *Feature Preview*
Consider adding a type constraint or validation logic to ensure half mode is
only applicable when donut is true. For example, you could use a discriminated
union type:
```typescript
export type EchartsPieFormData = QueryFormData &
LegendFormData & {
colorScheme?: string;
currentOwnValue?: string[] | null;
defaultValue?: string[] | null;
groupby: QueryFormColumn[];
innerRadius: number;
labelLine: boolean;
labelType: EchartsPieLabelType;
labelTemplate: string | null;
labelsOutside: boolean;
metric?: string;
outerRadius: number;
showLabels: boolean;
numberFormat: string;
dateFormat: string;
showLabelsThreshold: number;
roseType: 'radius' | 'area' | null;
thresholdForOther: number;
} & (
| { donut: false; half?: never }
| { donut: true; half: boolean }
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9613c66f-b9e1-47eb-8c4c-ae9b19698a7d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9613c66f-b9e1-47eb-8c4c-ae9b19698a7d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9613c66f-b9e1-47eb-8c4c-ae9b19698a7d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9613c66f-b9e1-47eb-8c4c-ae9b19698a7d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9613c66f-b9e1-47eb-8c4c-ae9b19698a7d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f7bec378-2ae5-4973-abf3-4c811bb2f011 -->
[](f7bec378-2ae5-4973-abf3-4c811bb2f011)
##########
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:
### Half-donut positioning overridden by padding logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The half-donut total value positioning logic is overridden when
chartPadding.top exists, causing the half-specific positioning to be lost.
###### Why this matters
When chartPadding.top is present, the second condition will always override
the half-specific positioning calculated in the first condition, resulting in
incorrect total value placement for half-donuts.
###### Suggested change ∙ *Feature Preview*
Combine the conditions or restructure the logic to preserve half-donut
positioning:
```typescript
if (chartPadding.top) {
padding.top = donut
? `${getDonutBase() + (chartPadding.top / height / 2) * 100}%`
: `${(chartPadding.top / height) * 100}%`;
} else if (half) {
padding.top = donut
? `${69 + (chartPadding.top / height / 2) * 100}%`
: `${(chartPadding.top / height) * 100}%`;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf26663-1846-4ead-9243-12c2cbf6dc1d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf26663-1846-4ead-9243-12c2cbf6dc1d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf26663-1846-4ead-9243-12c2cbf6dc1d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf26663-1846-4ead-9243-12c2cbf6dc1d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dcf26663-1846-4ead-9243-12c2cbf6dc1d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6869cfce-9862-4ee9-925b-6cba36d937c8 -->
[](6869cfce-9862-4ee9-925b-6cba36d937c8)
--
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]
