korbit-ai[bot] commented on code in PR #33634:
URL: https://github.com/apache/superset/pull/33634#discussion_r2116716564
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:
##########
@@ -196,6 +199,23 @@ function createCustomizeSection(
},
},
],
+ [
+ {
+ name: `only_total${controlSuffix}`,
+ config: {
+ type: 'CheckboxControl',
+ label: t('Only Total'),
+ default: true,
+ renderTrigger: true,
+ description: t(
+ 'Only show the total value on the stacked chart, and not show on
the selected category',
+ ),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.show_value?.value) &&
+ Boolean(controls?.stack?.value),
Review Comment:
### Redundant Type Conversion <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unnecessary Boolean conversions in visibility function that runs frequently
###### Why this matters
The double Boolean conversion is redundant since the AND operation (&&)
already returns a boolean. This function runs on every visibility check.
###### Suggested change ∙ *Feature Preview*
Simplify the boolean logic:
```typescript
visibility: ({ controls }: ControlPanelsContainerProps) =>
!!controls?.show_value?.value && !!controls?.stack?.value,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/db1fc55f-95f6-4fd4-b54e-c63749d9cf39/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/db1fc55f-95f6-4fd4-b54e-c63749d9cf39?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/db1fc55f-95f6-4fd4-b54e-c63749d9cf39?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/db1fc55f-95f6-4fd4-b54e-c63749d9cf39?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/db1fc55f-95f6-4fd4-b54e-c63749d9cf39)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:90df126d-52c5-45e8-bd9f-026cb64ad887 -->
[](90df126d-52c5-45e8-bd9f-026cb64ad887)
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -223,18 +229,42 @@ export default function transformProps(
}
const rebasedDataA = rebaseForecastDatum(data1, verboseMap);
+ const { totalStackedValues, thresholdValues } = extractDataTotalValues(
+ rebasedDataA,
+ {
+ stack,
+ percentageThreshold,
+ xAxisCol: xAxisLabel,
+ },
+ );
const MetricDisplayNameA = getMetricDisplayName(metrics[0], verboseMap);
const MetricDisplayNameB = getMetricDisplayName(metricsB[0], verboseMap);
- const [rawSeriesA] = extractSeries(rebasedDataA, {
+ const [rawSeriesA, sortedTotalValuesA] = extractSeries(rebasedDataA, {
fillNeighborValue: stack ? 0 : undefined,
xAxis: xAxisLabel,
+ sortSeriesType,
+ sortSeriesAscending,
+ stack,
+ totalStackedValues,
});
Review Comment:
### Redundant Series Extraction Configuration <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The extractSeries configuration object is duplicated with minor variations
between rawSeriesA and rawSeriesB extractions, making the code harder to follow.
###### Why this matters
Code duplication with slight variations increases cognitive load as readers
need to carefully compare the two blocks to understand the differences.
###### Suggested change ∙ *Feature Preview*
Extract common configuration into a base object and extend for differences:
```typescript
const baseSeriesConfig = {
xAxis: xAxisLabel,
fillNeighborValue: 0,
};
const seriesConfigA = {
...baseSeriesConfig,
fillNeighborValue: stack ? 0 : undefined,
sortSeriesType,
sortSeriesAscending,
stack,
totalStackedValues,
};
const seriesConfigB = {
...baseSeriesConfig,
fillNeighborValue: stackB ? 0 : undefined,
sortSeriesType: sortSeriesTypeB,
sortSeriesAscending: sortSeriesAscendingB,
stack: Boolean(stackB),
totalStackedValues: totalStackedValuesB,
};
const [rawSeriesA, sortedTotalValuesA] = extractSeries(rebasedDataA,
seriesConfigA);
const [rawSeriesB, sortedTotalValuesB] = extractSeries(rebasedDataB,
seriesConfigB);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22f0c630-537f-4b18-bff0-5132a9a955fc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22f0c630-537f-4b18-bff0-5132a9a955fc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22f0c630-537f-4b18-bff0-5132a9a955fc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22f0c630-537f-4b18-bff0-5132a9a955fc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22f0c630-537f-4b18-bff0-5132a9a955fc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:278749c6-d94e-448b-a183-44a84f70f22d -->
[](278749c6-d94e-448b-a183-44a84f70f22d)
--
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]
