sadpandajoe commented on code in PR #44588:
URL: https://github.com/apache/superset/pull/44588#discussion_r4126406187
##########
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:
@/tmp/reply_body.txt
##########
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:
Closing the loop on the ordinal-values point (`Low/Medium/High`, weekday
names, etc.) — this landed in ac0301d3, pushed before @sfirke's approval but
not called out explicitly in this thread, which is on me.
That commit adds an explicit `'none'` choice ("No sort (original order)") to
the Sort Y-axis control (`controlPanel.tsx`). Selecting it persists a non-null
value that `transformProps.ts` maps to the original query order, so it's
distinguishable from the unset/cleared state (which still falls back to
`alpha_asc`, per the reasoning above about the UI/chart-behavior mismatch
otherwise). Users with ordinal Y-axis values now have a recoverable, explicit
opt-out instead of being permanently locked into alphabetical order. Covered by
a new test in `transformProps.test.ts`.
Also just pushed the missing `messages.pot` entry for the new "No sort
(original order)" string so babel-extract passes.
--
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]
