rusackas commented on code in PR #43113:
URL: https://github.com/apache/superset/pull/43113#discussion_r3964533193
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1417,6 +1417,27 @@ 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.
+ const getSeriesFormatter = (seriesKey: string) =>
Review Comment:
Looks like this already got handled, 6fc91c2881a switched the derived-row
check to `getTimeOffset` instead of exact name matching, and the later
Ratio/label_map fixes built on top of it. There's a grouped-Percentage test
covering exactly this case now.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -2625,3 +2625,186 @@ describe('tooltip for metrics whose labels end in
forecast suffixes', () => {
expect(html).toContain('>ci<');
});
});
+test('tooltip formats each series with its own metric format instead of the
default formatter', () => {
+ // Two saved metrics with different formats: `pct_change` carries a
percentage
+ // D3 format, `count` carries a currency format. The series labels already
+ // honor each metric's format; the tooltip must do the same.
+ const chartProps = createTestChartProps({
+ formData: {
+ metrics: ['count', 'pct_change'],
+ richTooltip: true,
+ },
+ queriesData: [
+ createTestQueryData(
+ [{ count: 1000, pct_change: 0.1234, __timestamp: BASE_TIMESTAMP }],
+ { label_map: { count: ['count'], pct_change: ['pct_change'] } },
Review Comment:
We deliberately keep tests flat and self-contained here rather than sharing
setup across cases, per the repo's testing guidelines
(avoid-nesting-when-testing style). Some repetition between the
currency/Percentage/Ratio tests is the tradeoff for each one being readable on
its own, so I'd leave this as is.
--
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]
