sadpandajoe commented on code in PR #43314:
URL: https://github.com/apache/superset/pull/43314#discussion_r3919435938
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1378,6 +1471,9 @@ export default function transformProps(
grid: {
...defaultGrid,
...padding,
+ // Compact charts prioritize a viable coordinate system over keeping
+ // axis labels inside an already constrained grid rectangle.
+ containLabel: !usesCompactLayout,
Review Comment:
Intentional, not an oversight. This is unconditional by design, for the same
reason it's unconditional in `getViableTimeseriesEchartOptions`
(transformers.ts:874-886): once a chart is compact, we can't know ahead of time
how much space `containLabel: true` will reserve, since that depends on the
actual label text (formatter output, rotation, locale, longest tick) which
varies per query and isn't knowable at layout time. If we only disabled it when
our clamped top/bottom reservations exceed `reservationBudget`, a compact chart
with no explicit overrides could still hand ECharts an unbounded budget via
`containLabel: true`, which can shrink the computed plot rect below
`MIN_ECHARTS_GRID_HEIGHT` (or to zero) for long/rotated labels - which is
exactly the collapse this PR (and 2f0d78d) is trying to prevent. Forcing it off
for all compact charts trades 'labels may clip or overlap at the very edge' for
'the plot area is always guaranteed to be viable', which is the safer failure
mode at
these heights. The added SSR/unit test ('keeps a no-legend grid within a very
small post-control body') pins that guarantee.\n\nSo yes, we're intentionally
doing this for all compact charts rather than only when reservations exceed
budget - happy to revisit if there's a case where the label-overlap tradeoff is
worse than the collapse it prevents, but I'd want a concrete repro first since
narrowing this reintroduces the original bug for charts with long tick labels.
--
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]
