EnxDev commented on code in PR #43339:
URL: https://github.com/apache/superset/pull/43339#discussion_r3881132521
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1260,25 +1279,38 @@ export default function transformProps(
// have less overlap, so disabling hideOverlap is safe.
// At 0° rotation, also disable hideOverlap when showMaxLabel
// is active so the forced boundary label is never suppressed
- // by ECharts' overlap detection (#39899).
- hideOverlap: showMaxLabel
- ? false
- : !(xAxisType === AxisType.Time && xAxisLabelRotation !== 0),
+ // by ECharts' overlap detection (#39899). Pinned ticks label
+ // every bucket, which does crowd, so thinning always wins there.
+ hideOverlap:
+ !!temporalTickValues ||
+ (showMaxLabel
+ ? false
+ : !(xAxisType === AxisType.Time && xAxisLabelRotation !== 0)),
formatter: deduplicatedFormatter,
rotate: xAxisLabelRotation,
interval: xAxisLabelInterval,
// Force the boundary labels on non-rotated time axes so the first
// and last dates stay visible: hideOverlap can hide the last label,
// and a min date that falls between "nice" ticks otherwise renders
// no beginning label. Skipped when rotated to avoid phantom labels
- // at the axis boundary.
- ...(showMaxLabel && {
- showMaxLabel: true,
- alignMaxLabel: 'right',
- showMinLabel: true,
- alignMinLabel: 'left',
- }),
+ // at the axis boundary. Also skipped for pinned ticks: the boundary
+ // buckets are already real ticks there, and showMaxLabel only shields
+ // its immediate neighbour — hideOverlap can still drop it against a
+ // farther label on a crowded weekly axis, reopening #39899.
+ ...(showMaxLabel &&
Review Comment:
Real risk, agreed — reverted that guard. `showMaxLabel`/`showMinLabel` are
now applied on pinned axes too, alongside `hideOverlap: true`. It only shields
the immediate neighbour, not the whole axis, but that's the same guarantee
unpinned axes get and is strictly better than the no-protection state this
thread flagged. Updated the two regression tests that had asserted the old
(unprotected) behavior. Landed in 6dad5c9390.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -762,6 +764,22 @@ export default function transformProps(
const { setDataMask = () => {}, onContextMenu } = hooks;
const alignTicks = yAxisIndex !== yAxisIndexB;
+ // Weekly grains: pin the ticks to the buckets. Both queries share the axis.
+ // Skipped when a timeseries annotation is shown: it widens the axis past the
+ // buckets and ECharts clips pinned ticks to the extent, leaving that span
bare.
+ const hasTimeseriesAnnotation = annotationLayers.some(
Review Comment:
Agreed — extracted a `getTemporalAxisTickConfig()` helper in `series.ts`
that returns the axisLabel/axisTick fragment; both files now spread it into
their `xAxis` object instead of duplicating the block. That also made the
cap/label-mismatch fix above a one-place change instead of two. Landed in
6dad5c9390.
--
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]
