msyavuz commented on code in PR #43339:
URL: https://github.com/apache/superset/pull/43339#discussion_r3880251234
##########
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:
`hideOverlap` thins labels independently of `capTickMarks`' `index % step
=== 0` subset, so surviving labels can land with no tick or gridline under them
(100 buckets → step 2, ticks on even indices, labels typically at 0/5/10/15…).
`splitLine` follows `axisTick.customValues` too, since `createAxisTicks` calls
`axis.getTickModel()` rather than the model passed to it.
##########
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:
This block is near-verbatim with Timeseries and only the data argument
differs — worth one helper returning the axis fragment, given the tick/label
fix above now has to land in two places?
##########
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 cap/label mismatch here.
##########
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:
Pinned axes now drop `showMaxLabel` and force `hideOverlap: true`, where a
weekly 0° rotation axis previously got `showMaxLabel: true` + `hideOverlap:
false` — doesn't that let the last bucket's label be hidden again, which is the
#39899 symptom?
--
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]
