WhoamiI00 commented on code in PR #43113:
URL: https://github.com/apache/superset/pull/43113#discussion_r3912572624
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1435,6 +1449,29 @@ export default function transformProps(
value.forecastTrend || value.forecastLower || value.forecastUpper,
);
+ // Resolve the value formatter per series so each metric keeps its own
+ // D3/currency format, matching how the series labels are formatted.
+ // Without the series key, `getCustomFormatter` returns undefined for
+ // multi-metric charts and every row falls back to `defaultFormatter`,
+ // rendering the y-axis/currency format for all metrics.
+ //
+ // The tooltip key is the rendered series name, so resolve it through
+ // `labelMap`, whose values lead with the raw metric label. Series
+ // renamed by a verbose_name are absent from that map, so fall back to
+ // the verbose-name inversion, as MixedTimeseries does. A Percentage
+ // comparison row is a ratio rather than a value in the metric's units,
+ // so it takes the percent formatter instead of the metric's own
format.
+ const getSeriesFormatter = (seriesKey: string) =>
+ forcePercentFormatter || isPercentageComparisonSeries(seriesKey)
Review Comment:
Confirmed and fixed in 83e4fe0c — you're right, and it was the loose end I
flagged earlier rather than a new one, so thanks for pushing on it.
Reproduced first: a currency metric with `comparison_type: Ratio` and a `1
week ago` offset rendered the derived row as `$ 1.25`.
One thing I did differently from a literal reading of the suggestion: Ratio
does **not** go through the percent formatter. The backend computes it as `s_df
/ c_df` (`superset/utils/pandas_postprocessing/compare.py`), so the value is a
plain multiplier — 1.25 means "25% higher", and rendering it as `125.00%` would
be a different wrong answer. It gets a unitless number format instead:
```ts
const isDerivedComparisonSeries = (seriesKey: string) =>
array.includes(seriesKey) ||
getTimeOffset({ name: seriesKey }, array) !== undefined;
const getComparisonFormatter = (seriesKey: string) => {
if (!isDerivedComparisonSeries(seriesKey)) {
return undefined;
}
switch (chartProps.rawFormData?.comparison_type) {
case ComparisonType.Percentage:
return percentFormatter;
case ComparisonType.Ratio:
return ratioFormatter;
default:
return undefined;
}
};
```
The series-matching half is unchanged and now serves both — the split is
between *recognising* a derived comparison row and *choosing the formatter* for
it, so Percentage and Ratio share the matching and differ only in output.
Three fixtures added:
- Ratio on a currency metric renders `1.25`, not `$ 1.25`
- the same grouped (`1 week ago, East`), since that was the gap you found
last time
- **Difference keeps the currency** — `s - c` is still in the metric's
units, so it must stay on the metric's own format
Both Ratio tests fail on the previous commit with `Expected substring: not
"$ 1.25"`. The Difference one passes either way by design: it pins the boundary
so the three comparison types cannot drift into one bucket later.
120 tests green in `Timeseries/transformProps.test.ts` — up from 101 because
the branch picked up master's newer tests when it was updated.
--
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]
