sadpandajoe commented on code in PR #43669:
URL: https://github.com/apache/superset/pull/43669#discussion_r3896322014
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1205,46 +1207,24 @@ export default function transformProps(
// When showMaxLabel is true, ECharts may render a label at the axis
// boundary that formats identically to the last data-point tick (e.g.
- // "2005" appears twice with Year grain). Wrap the formatter to suppress
- // consecutive duplicate labels.
+ // "2005" appears twice with Year grain), and hideOverlap must stay off so
+ // that forced boundary label is never suppressed (#39899). Wrap the
+ // formatter to suppress consecutive duplicate labels and to thin out
+ // labels that would otherwise visually collide, since hideOverlap can no
+ // longer do that for us.
const showMaxLabel =
xAxisType === AxisType.Time &&
xAxisLabelRotation === 0 &&
!!resolvedTimeGrain;
const deduplicatedFormatter = showMaxLabel
- ? (() => {
- let lastLabel: string | undefined;
- let lastValue: number | undefined;
- const wrapper = (value: number | string) => {
- // ECharts formats the labels in repeated ascending passes. Reset the
- // dedup state when the sequence restarts so a forced boundary label
- // (e.g. the min date) isn't blanked by the previous pass's last
label
- // when both format identically (e.g. a May-to-May range).
- if (
- typeof value === 'number' &&
- lastValue !== undefined &&
- value <= lastValue
- ) {
- lastLabel = undefined;
- }
- if (typeof value === 'number') {
- lastValue = value;
- }
- const label =
- typeof xAxisFormatter === 'function'
- ? (xAxisFormatter as Function)(value)
- : String(value);
- if (label === lastLabel) {
- return '';
- }
- lastLabel = label;
- return label;
- };
- if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) {
- (wrapper as any).id = (xAxisFormatter as any).id;
- }
- return wrapper;
- })()
+ ? createSpacedXAxisFormatter(
+ xAxisFormatter,
+ ...getXAxisDomain(
+ [rebasedData as Record<string, unknown>[]],
+ xAxisLabel,
+ ),
+ Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0),
+ )
Review Comment:
Acknowledged — per rebenitez1802's review, this is a real but acceptable
heuristic. Cheap improvement noted: swap the fixed `width - 2 * gridOffsetLeft`
for the already-computed `padding.left`/`padding.right` insets. Leaving as a
follow-up rather than blocking this fix.
##########
superset-frontend/plugins/plugin-chart-echarts/src/constants.ts:
##########
@@ -52,6 +52,12 @@ export const TIMESERIES_CONSTANTS = {
microChartHeight: 60,
// One y-axis tick per this many pixels of chart height
yAxisPixelsPerTick: 80,
+ // Rough average glyph width (px) used to estimate whether adjacent x-axis
+ // time labels would visually collide, since the real rendered width isn't
+ // known until ECharts lays out the axis.
+ xAxisLabelCharWidthPx: 7,
+ // Minimum gap (px) to keep between adjacent x-axis time labels.
+ xAxisLabelMinGapPx: 8,
Review Comment:
Acknowledged — per rebenitez1802's review, the fixed 7px/char estimate is a
real but acceptable heuristic (worst case still beats no suppression). A
measureText-based estimate would be a nice follow-up, not blocking this fix.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:
##########
@@ -213,3 +215,105 @@ export function getXAxisFormatter(
}
return String;
}
+
+type XAxisFormatterFn =
+ | TimeFormatter
+ | NumberFormatter
+ | StringConstructor
+ | ((value: number | string) => string);
+
+/**
+ * Wraps an x-axis time formatter so that:
+ * - consecutive ticks that format to identical text are blanked (e.g. the
+ * boundary label forced by showMaxLabel duplicating the last real tick).
+ * - ticks that would render close enough to visually collide with the
+ * previously shown label are blanked, since disabling ECharts'
+ * `hideOverlap` (required to keep the forced boundary label visible, see
+ * #39899) also disables its native overlap suppression for every other
+ * label on the axis.
+ *
+ * The forced axis boundary labels (domainMin/domainMax) are never blanked by
+ * the spacing check so they stay visible regardless of density.
+ */
+export function createSpacedXAxisFormatter(
+ xAxisFormatter: XAxisFormatterFn | undefined,
+ domainMin: number | undefined,
+ domainMax: number | undefined,
+ plotWidthPx: number,
+): (value: number | string) => string {
+ const pixelsPerMs =
+ domainMin !== undefined && domainMax !== undefined && domainMax > domainMin
+ ? plotWidthPx / (domainMax - domainMin)
+ : undefined;
+ let lastLabel: string | undefined;
+ let lastValue: number | undefined;
+ let lastShownValue: number | undefined;
+ const wrapper = (value: number | string) => {
+ // ECharts formats the labels in repeated ascending passes. Reset the
+ // dedup/spacing state when the sequence restarts so a forced boundary
+ // label (e.g. the min date) isn't blanked by the previous pass's state
+ // when both format identically (e.g. a May-to-May range).
+ if (
+ typeof value === 'number' &&
+ lastValue !== undefined &&
+ value <= lastValue
+ ) {
+ lastLabel = undefined;
+ lastShownValue = undefined;
+ }
+ if (typeof value === 'number') {
+ lastValue = value;
+ }
+ const label =
+ typeof xAxisFormatter === 'function'
+ ? (xAxisFormatter as Function)(value)
+ : String(value);
+ if (label === lastLabel) {
+ return '';
+ }
+ const isBoundary =
+ typeof value === 'number' && (value === domainMin || value ===
domainMax);
+ if (
+ !isBoundary &&
+ typeof value === 'number' &&
+ pixelsPerMs !== undefined &&
+ lastShownValue !== undefined &&
+ (value - lastShownValue) * pixelsPerMs <
+ label.length * TIMESERIES_CONSTANTS.xAxisLabelCharWidthPx +
+ TIMESERIES_CONSTANTS.xAxisLabelMinGapPx
+ ) {
+ return '';
+ }
+ lastLabel = label;
+ if (typeof value === 'number') {
+ lastShownValue = value;
+ }
+ return label;
+ };
+ if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) {
+ (wrapper as { id?: unknown }).id = (xAxisFormatter as { id?: unknown }).id;
+ }
+ return wrapper;
+}
+
+/**
+ * Computes the [min, max] of a temporal x-axis column across one or more
+ * data record arrays, for use with createSpacedXAxisFormatter.
+ */
+export function getXAxisDomain(
+ dataRecordArrays: Record<string, unknown>[][],
+ xAxisCol: string,
+): [number | undefined, number | undefined] {
+ let domainMin: number | undefined;
+ let domainMax: number | undefined;
+ dataRecordArrays.forEach(records => {
+ records.forEach(record => {
+ const value = record[xAxisCol];
+ if (typeof value === 'number') {
+ if (domainMin === undefined || value < domainMin) domainMin = value;
+ if (domainMax === undefined || value > domainMax) domainMax = value;
+ }
+ });
Review Comment:
Acknowledged — per rebenitez1802's review, this looks like a false positive
on the standard pipeline: the chart-data API serializes temporal columns to
epoch-ms numbers, so the numeric-only filter in getXAxisDomain is correct
there. Not blocking this fix.
--
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]
