EnxDev commented on code in PR #43193:
URL: https://github.com/apache/superset/pull/43193#discussion_r3966288186
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1204,6 +1205,26 @@ export default function transformProps(
xAxisType === AxisType.Time &&
xAxisLabelRotation === 0 &&
!!resolvedTimeGrain;
+ const {
+ interval: xAxisLabelIntervalValue,
+ showAllLabels: showAllXAxisLabels,
+ } = getAxisLabelInterval(xAxisLabelInterval);
+ // axisLabel.interval is only ever consulted for category axes in ECharts
+ // (axisTickLabelBuilder routes to makeCategoryLabels there; a time axis
+ // goes through makeRealNumberLabels and never reads it), and Superset sets
+ // xAxis.type to Time whenever the x-axis column is temporal -- the common
+ // case #36325 actually reports. On a time axis, tick density is governed
+ // by minInterval/maxInterval instead, and minInterval alone only floors
+ // the spacing: ECharts can still choose a wider "nice" interval to fit the
+ // available width. Pinning both bounds to the resolved grain forces one
+ // tick, and therefore one label, per data point.
+ const timeGrainIntervalMs = resolvedTimeGrain
+ ? (TIMEGRAIN_TO_TIMESTAMP[
+ resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
+ ] ?? undefined)
+ : undefined;
+ const showAllOnTimeAxis =
+ showAllXAxisLabels && xAxisType === AxisType.Time && !!timeGrainIntervalMs;
Review Comment:
**[P1] Keep `All` working for every supported time grain.**
`TIMEGRAIN_TO_TIMESTAMP` only contains hour, day, month, quarter, and year, so
`P1W` and the anchored weekly grains (as well as second/minute grains) resolve
to `undefined`. That leaves `showAllOnTimeAxis` false and the time axis on its
old automatic tick selection. The linked issue explicitly reports Week:
rendering 52 weekly points with these resulting options gives 13 month-aligned
ticks, not 52 labels. Could we derive the ticks from the actual timestamps
instead and add a `TimeGranularity.WEEK` regression?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1273,17 +1298,19 @@ export default function transformProps(
}),
},
minorTick: { show: minorTicks },
+ // "All" wins over forceMaxInterval's own (opposite) request, since the
+ // user explicitly asked to see every label.
minInterval:
- xAxisType === AxisType.Time && resolvedTimeGrain && !forceMaxInterval
- ? (TIMEGRAIN_TO_TIMESTAMP[
- resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
- ] ?? 0)
+ xAxisType === AxisType.Time && resolvedTimeGrain
+ ? showAllOnTimeAxis || !forceMaxInterval
+ ? (timeGrainIntervalMs ?? 0)
+ : 0
: 0,
maxInterval:
- xAxisType === AxisType.Time && resolvedTimeGrain && forceMaxInterval
- ? TIMEGRAIN_TO_TIMESTAMP[
- resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
- ]
+ xAxisType === AxisType.Time && resolvedTimeGrain
+ ? showAllOnTimeAxis || forceMaxInterval
+ ? timeGrainIntervalMs
Review Comment:
**[P1] Avoid generating a tick for every empty grain bucket.**
`minInterval`/`maxInterval` constrain ECharts' continuous time scale; they do
not mean one tick per data point. For example, two daily points at 2010-01-01
and 2020-01-01 produce 2,898 ticks here and repeated `Exceed safe limit in
TimeScale["getTicks"]` warnings, instead of two labels. Gaps are common in
grouped time data, so `All` can make rendering scale with the date range and
add labels where there is no point. Could we pass the actual timestamp values
through ECharts 6's `axisLabel.customValues`/`axisTick.customValues` (possibly
extending `resolveTemporalTickValues` from current `master`) and cover a sparse
range in a test?
--
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]
