WhoamiI00 commented on code in PR #43113:
URL: https://github.com/apache/superset/pull/43113#discussion_r3823710312
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1417,6 +1417,22 @@ 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.
+ const getSeriesFormatter = (seriesKey: string) =>
+ forcePercentFormatter
+ ? percentFormatter
+ : (getCustomFormatter(
+ customFormatters,
+ metrics,
+ labelMap?.[seriesKey]?.[0],
+ ) ?? defaultFormatter);
Review Comment:
Good catch, and confirmed — that was a real gap. Fixed in 41f91d37.
Reproduced it first: a currency metric with `comparison_type: Percentage`
and a `1 week ago` offset rendered the derived row as `$ 0.25` in the tooltip.
The per-series lookup resolved that row back to the source metric, so it picked
up the metric's saved `CurrencyFormatter` even though the value is a ratio.
Those rows now take the percent formatter instead, so the same fixture
renders `$ 100` for the source metric and `25.00%` for the comparison row. The
offsets come from `rawFormData.time_compare`, and the labels follow
`renameOperator` — the offset alone, or `<metric>, <offset>` when several
metrics are plotted.
Added the fixture you asked for; it fails without the change with `Expected
substring: "25.00%"`.
One thing I left alone deliberately: `Ratio` is unitless too, so it has the
same problem, but the right output there is a plain number rather than a
percent, and that felt like your call rather than mine. Happy to fold it in if
you want it in this PR.
Also rebuilt the branch on current `master` — it had gone conflicting
against the new tooltip-truncation tests, which sit exactly where these tests
were appended. `Timeseries` + `MixedTimeseries` are green at 309 tests.
--
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]
