EnxDev commented on code in PR #43339:
URL: https://github.com/apache/superset/pull/43339#discussion_r3881131029
##########
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 &&
+ !temporalTickValues && {
+ showMaxLabel: true,
+ alignMaxLabel: 'right',
+ showMinLabel: true,
+ alignMinLabel: 'left',
+ }),
+ ...(temporalTickValues && { customValues: temporalTickValues }),
},
+ // Gridlines follow axisTick.customValues; cap it so a long weekly
+ // range doesn't comb.
+ ...(temporalTickValues && {
+ axisTick: { customValues: capTickMarks(temporalTickValues) },
Review Comment:
Fixed — both now share one capped set via a new
`getTemporalAxisTickConfig()` helper in `series.ts` (also used for the
MixedTimeseries case below and the duplication cleanup).
`axisLabel.customValues`/`axisTick.customValues` are both
`capTickMarks(temporalTickValues)` now, so a label that survives `hideOverlap`
thinning always has a real tick and gridline under it. Added a regression test
(261 buckets, cap=60) asserting `axisLabel.customValues` equals
`axisTick.customValues`. Landed in 6dad5c9390.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -774,19 +792,34 @@ export default function transformProps(
nameGap: xAxisTitleMarginPx,
nameLocation: 'middle',
axisLabel: {
- hideOverlap: showMaxLabel
- ? false
- : !(xAxisType === AxisType.Time && xAxisLabelRotation !== 0),
+ // Pinned ticks label every bucket, so keep thinning on even when the
+ // showMaxLabel/rotation branch would otherwise drop it.
+ hideOverlap:
+ !!temporalTickValues ||
+ (showMaxLabel
+ ? false
+ : !(xAxisType === AxisType.Time && xAxisLabelRotation !== 0)),
formatter: deduplicatedFormatter,
rotate: xAxisLabelRotation,
interval: xAxisLabelInterval,
- ...(showMaxLabel && {
- showMaxLabel: true,
- alignMaxLabel: 'right',
- showMinLabel: true,
- alignMinLabel: 'left',
- }),
+ // 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 &&
+ !temporalTickValues && {
+ showMaxLabel: true,
+ alignMaxLabel: 'right',
+ showMinLabel: true,
+ alignMinLabel: 'left',
+ }),
+ ...(temporalTickValues && { customValues: temporalTickValues }),
},
+ // Gridlines follow axisTick.customValues; cap it so a long weekly
+ // range doesn't comb.
+ ...(temporalTickValues && {
+ axisTick: { customValues: capTickMarks(temporalTickValues) },
Review Comment:
Same fix — MixedTimeseries now goes through the shared
`getTemporalAxisTickConfig()` helper too, so this is resolved along with the
Timeseries case above. 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]
