sadpandajoe commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r4128539545
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -422,17 +424,23 @@ export default function transformProps(
let displayName: string;
- if (groupby.length > 0) {
- // When we have groupby, format as "metric, dimension"
- const metricPart: string = showQueryIdentifiers
- ? `${MetricDisplayNameA} (Query A)`
- : MetricDisplayNameA;
- displayName = entryName.includes(metricPart)
- ? entryName
- : `${metricPart}, ${entryName}`;
+ if (truncateMetric && groupby.length > 0) {
+ const groupbyValues = labelMap?.[seriesName] || [];
Review Comment:
When both `truncateMetric` and `truncateMetricB` are enabled and Query A and
Query B share a groupby value, both series get the exact same truncated name
(e.g. both render as `boy`), because this branch never applies the Query
A/Query B distinction that the untruncated path just below still uses — so the
two series become indistinguishable in the legend and can't be toggled
independently. This happens even with `showQueryIdentifiers` off, so it isn't
limited to the identifier case already discussed above. None of the four new
tests cover Query A and Query B sharing a groupby value. Could the truncated
name retain some query marker when both queries collide on the same groupby
value?
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts:
##########
@@ -136,6 +138,8 @@ export const DEFAULT_FORM_DATA:
EchartsMixedTimeseriesFormData = {
zoomable: TIMESERIES_DEFAULTS.zoomable,
richTooltip: TIMESERIES_DEFAULTS.richTooltip,
showQueryIdentifiers: false,
+ truncateMetric: false,
+ truncateMetricB: false,
Review Comment:
Agreed—for a chart whose saved form data omits the new
`truncateMetric`/`truncateMetricB` keys (e.g. a legacy chart created before
this field existed), the runtime default here is `false`, but the control
checkbox itself defaults to checked. Re-saving that chart from Explore would
silently flip its rendered series names by picking up the control's `true`
default that the transform layer never assumed. Should `DEFAULT_FORM_DATA`
match the control's default instead?
--
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]
