Copilot commented on code in PR #36896:
URL: https://github.com/apache/superset/pull/36896#discussion_r2673619162
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -506,6 +511,11 @@ export default function transformProps(
showValue: showValueB,
onlyTotal: onlyTotalB,
stack: Boolean(stackB),
+ // For grouped+stacked: when we have multiple groupby dimensions,
+ // use the first dimension value as stackGroup to group related bars
+ stackGroup: groupbyB.length > 1
+ ? String(labelMapB?.[seriesName]?.[0] || entryName.split(', ')[0])
Review Comment:
The fallback logic `entryName.split(', ')[0]` assumes that entry names
always use comma-space as a delimiter between dimensions. If the label_map
lookup fails and the entry name format differs, this could produce incorrect
stack groups. Consider adding a comment explaining when this fallback is
expected to be used, or add validation to ensure the split produces the
expected result.
```suggestion
// use the first dimension value as stackGroup to group related bars.
// If labelMapB lookup fails, fall back to entryName. When entryName
// encodes multiple dimensions as "dim1, dim2, ...", use the first
// segment; otherwise, use the full entryName without splitting.
stackGroup: groupbyB.length > 1
? String(
labelMapB?.[seriesName]?.[0] ??
(entryName.includes(', ')
? entryName.split(', ')[0]
: entryName),
)
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -434,6 +434,11 @@ export default function transformProps(
showValue,
onlyTotal,
stack: Boolean(stack),
+ // For grouped+stacked: when we have multiple groupby dimensions,
+ // use the first dimension value as stackGroup to group related bars
+ stackGroup: groupby.length > 1
+ ? String(labelMap?.[seriesName]?.[0] || entryName.split(', ')[0])
+ : undefined,
Review Comment:
The fallback logic `entryName.split(', ')[0]` assumes that entry names
always use comma-space as a delimiter between dimensions. If the label_map
lookup fails and the entry name format differs, this could produce incorrect
stack groups. Consider adding a comment explaining when this fallback is
expected to be used, or add validation to ensure the split produces the
expected result.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -178,6 +178,7 @@ export function transformSeries(
areaOpacity?: number;
seriesType?: EchartsTimeseriesSeriesType;
stack?: StackType;
+ stackGroup?: string; // For grouped+stacked charts: dimension value to
group by
Review Comment:
The comment for the stackGroup parameter could be more descriptive. Consider
expanding it to explain that when provided, stackGroup becomes the base stack
ID, allowing series with the same stackGroup value to stack together while
keeping different groups separate. For example: "For grouped+stacked charts:
the grouping dimension value (e.g., 'Sprint 1') used as the base stack ID.
Series with the same stackGroup will stack together, while different
stackGroups will appear side-by-side."
```suggestion
stackGroup?: string; // For grouped+stacked charts: the grouping
dimension value (e.g., 'Sprint 1') used as the base stack ID. Series with the
same stackGroup value will stack together, while different stackGroups will
appear side-by-side.
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -434,6 +434,11 @@ export default function transformProps(
showValue,
onlyTotal,
stack: Boolean(stack),
+ // For grouped+stacked: when we have multiple groupby dimensions,
+ // use the first dimension value as stackGroup to group related bars
+ stackGroup: groupby.length > 1
+ ? String(labelMap?.[seriesName]?.[0] || entryName.split(', ')[0])
+ : undefined,
Review Comment:
The new stackGroup feature for grouped+stacked charts lacks test coverage.
Consider adding a test case that verifies the behavior when groupby.length > 1,
including checking that the stackGroup is correctly extracted from labelMap and
that series are assigned the expected stack IDs for proper grouping and
stacking behavior.
--
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]
