sadpandajoe commented on code in PR #43238:
URL: https://github.com/apache/superset/pull/43238#discussion_r3994365186
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -480,11 +480,62 @@ export default function transformProps(
? [minMarkerSize, maxMarkerSize]
: [maxMarkerSize, minMarkerSize];
+ // When stackDimension is configured, each series is assigned to a separate
+ // ECharts stack group keyed by the dimension value. Compute this mapping
+ // before calling extractShowValueIndexes so each group's topmost series is
+ // tracked independently (fixing the bug where only the last series across
+ // all groups was flagged to show the total label).
+ // When metrics.length > 1 the label-map tuple is [metric, dim0, dim1, ...],
+ // so the stackDimension sits at offset 1 + groupby.indexOf(stackDimension).
+ // When there is a single metric the tuple is [dim0, dim1, ...] with no
+ // metric prefix, so the offset is just groupby.indexOf(stackDimension).
+ const idxSelectedDimension =
+ stack === StackControlsValue.Stack &&
+ stackDimension &&
+ chartProps.rawFormData?.groupby
+ ? (formData.metrics && formData.metrics.length > 1 ? 1 : 0) +
+ chartProps.rawFormData.groupby.indexOf(stackDimension)
Review Comment:
Removing the selected “Split stack by” field leaves `stackDimension` in form
data, and `indexOf` returns -1; with multiple metrics the offset becomes 0, so
the metric label is used as the stack ID and the bars stop stacking together.
Should this require `groupby.includes(stackDimension)` (or clear the stale
control) before deriving the index?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -417,42 +417,70 @@ export function extractDataTotalValues(
};
}
+const DEFAULT_STACK_GROUP = '__default__';
+
+/**
+ * Computes, per stack group, which series index is the "topmost" (i.e. the
+ * series that should display the value label) for each data point.
+ *
+ * When a stackDimension splits bars into separate ECharts stack groups the
+ * computation must be done independently per group, otherwise only the
+ * globally-last series is flagged and all other groups miss their label.
+ *
+ * @param series The raw series array (parallel to the rendered series).
+ * @param opts.stack Whether stacking is active.
+ * @param opts.onlyTotal Whether to show only the stack total.
+ * @param opts.isHorizontal Whether the chart is horizontal.
+ * @param opts.legendState Active legend state (hidden series are skipped).
+ * @param opts.seriesStackIds Optional per-series stack-group key (parallel to
+ * `series`). Defaults to a single shared group.
+ * @returns A `Record<stackGroupKey, number[]>` where each inner array maps
+ * `dataIndex → seriesIndex` of the topmost series in that group.
+ */
export function extractShowValueIndexes(
series: SeriesOption[],
opts: {
stack: StackType;
onlyTotal?: boolean;
isHorizontal?: boolean;
legendState?: LegendState;
+ seriesStackIds?: string[];
},
-): number[] {
- const showValueIndexes: number[] = [];
- const { legendState, stack, isHorizontal, onlyTotal } = opts;
- if (stack) {
- series.forEach((entry, seriesIndex) => {
- const { data = [] } = entry;
- (data as [any, number][]).forEach((datum, dataIndex) => {
- if (entry.id && legendState && !legendState[entry.id]) {
- return;
- }
- if (!onlyTotal && datum[isHorizontal ? 0 : 1] !== null) {
+): Record<string, number[]> {
+ const result: Record<string, number[]> = {};
+ const { legendState, stack, isHorizontal, onlyTotal, seriesStackIds } = opts;
+ if (!stack) {
+ return result;
+ }
+
+ series.forEach((entry, seriesIndex) => {
+ const stackGroup = seriesStackIds?.[seriesIndex] ?? DEFAULT_STACK_GROUP;
+ if (!result[stackGroup]) {
Review Comment:
A dimension value such as `__proto__` or `constructor` resolves an inherited
property here instead of creating the group array; the accumulator then writes
through a prototype object, corrupting this stack’s total and potentially later
groups. Could both data-keyed records here and in `transformProps` use a `Map`
or null-prototype object so every dimension value is an own key?
--
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]
