msyavuz commented on code in PR #43339:
URL: https://github.com/apache/superset/pull/43339#discussion_r3845167808
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -986,6 +987,45 @@ export function getAxisType(
return AxisType.Category;
}
+/**
+ * Bucket timestamps a temporal axis should tick on, or undefined to let
ECharts
+ * choose.
+ *
+ * ECharts generates time ticks from a calendar ladder with no week unit, so
for
+ * weekly data it steps days from the 1st of each month instead: labels drift
+ * across weekdays and snap to month starts (#17226). Coarser grains already
land
+ * on their data and keep ECharts' calendar-nice labels.
+ */
+export function getTemporalTickValues(
+ data: DataRecord[],
+ xAxisLabel: string,
+ xAxisType: AxisType,
+ timeGrain?: string,
+): number[] | undefined {
+ if (
+ xAxisType !== AxisType.Time ||
+ !timeGrain ||
+ !WEEKLY_TIME_GRAINS.has(timeGrain)
+ ) {
+ return undefined;
+ }
+ const values = new Set<number>();
+ data.forEach(row => {
+ const value = row[xAxisLabel];
+ const timestamp =
+ // eslint-disable-next-line no-nested-ternary
+ value instanceof Date
+ ? value.getTime()
+ : typeof value === 'string'
+ ? new Date(value).getTime()
Review Comment:
`new Date(value)` parses a zone-less bucket string as UTC while echarts'
`parseDate` treats it as local, so at UTC+2 the point sits at `04-05T22:00Z`
and the pinned tick at `04-06T00:00Z` — does that offset the tick and clip the
boundary one? The new test uses `.toISOString()`, so it wouldn't catch this.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1243,6 +1244,23 @@ export default function transformProps(
})()
: xAxisFormatter;
+ // Weekly grains: pin the ticks to the buckets ECharts would otherwise miss.
+ // A timeseries annotation contributes its own timestamps and widens the axis
+ // past the buckets, and ECharts clips pinned ticks to the extent, so that
+ // span would render bare — leave those charts on ECharts' own ticks.
+ const hasTimeseriesAnnotation = annotationLayers.some(
Review Comment:
Any shown timeseries annotation disables pinning, even one whose records sit
entirely inside the bucket range and widen nothing — worth comparing its extent
to the buckets instead of gating on layer type?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1258,10 +1276,13 @@ 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 ||
Review Comment:
`hideOverlap` is forced on for weekly here but `showMaxLabel: true` is still
emitted below — does that reopen #39899 for weekly grains, given `hideOverlap`
only drops the max label's immediate neighbour?
--
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]
