sadpandajoe commented on code in PR #44588:
URL: https://github.com/apache/superset/pull/44588#discussion_r4109167054
##########
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:
##########
@@ -266,10 +266,14 @@ export default function transformProps(
metricLabel,
xAxisColumnName,
);
+ // Unlike the X-axis (often a temporal column that already arrives from
+ // the backend in a natural, e.g. chronological, order), the Y-axis
+ // groupby dimension has no such natural ordering to fall back on, so
+ // default it to ascending sort when the user hasn't chosen one.
const sortedYAxisValues = sortAxisValues(
yAxisValues,
data,
- sortYAxis,
+ sortYAxis ?? 'alpha_asc',
Review Comment:
Thanks for digging into this — both alternatives were actually tried during
this fix's development and reverted for concrete reasons:
- Moving the default onto the `sort_y_axis` control (`controlPanel.tsx`)
doesn't actually preserve old rendering for existing charts either:
`dashboard/actions/hydrate.ts`'s `applyDefaultFormData` reuses the same
control-default machinery
(`getAllControlsState`/`applyMapStateToPropsToControl`) for every persisted
dashboard chart's form data, not just newly-created ones — so it silently
re-sorts existing charts too, just via a different code path. It also breaks
the control's own clear ("×") button within an Explore editing session: a
`clearable` control paired with a `default` can't distinguish "cleared" from
"never touched" once `getControlState.ts`'s loose `value == null` check
re-applies the default on every recompute.
- Aligning `buildQuery.ts`'s `ORDER BY` with the default would fix the
row-limit inconsistency you noticed, but it changes *which rows* survive
`row_limit` for a sparse Heatmap, not just their display order — a materially
different and larger risk than a render-order change. An earlier pass
considered exactly this and rejected it for that reason.
Still working through your point on ordinal-but-non-alphabetical Y-axis
values (`Low/Medium/High`, weekdays, etc.) — that's a sharper case than what we
originally scoped for and deserves a proper answer rather than folding it into
the general tradeoff. Will follow up on that separately.
--
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]
