EnxDev commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5339664912
## EnxDev's Review Agent โ apache/superset#43193 ยท HEAD 8e9c772
**comment** โ the category-axis half is correct and well-argued, but on a
temporal x-axis (the case in #36325) ECharts still ignores
`axisLabel.interval`, so "All" remains unhonored there.
### ๐ด Functional
-
**`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:1271`**
(and `MixedTimeseries/transformProps.ts:782`) ยท _High_ โ ECharts only reads
`axisLabel.interval` on **category** axes. In echarts 6.1.0,
`coord/axisTickLabelBuilder.js:81`:
```js
// Only ordinal scale support tick interval
return axis.type === 'category' ? makeCategoryLabels(axis, ctx) :
makeRealNumberLabels(axis);
```
and `coord/axisHelper.js:199` gates the same way:
```js
export function shouldShowAllLabels(axis) {
return axis.type === 'category' &&
getOptionCategoryInterval(axis.getLabelModel()) === 0;
}
```
Issue #36325 is an Area chart over a temporal x-axis with a day/week
grain, so `getAxisType` (`src/utils/series.ts:938`) returns `AxisType.Time` and
the newly-coerced `interval: 0` is dropped just as the string `'0'` was. On a
time axis the label set is `axis.scale.getTicks()` mapped 1:1
(`makeRealNumberLabels`), bounded by the `minInterval`/`maxInterval` this file
already derives from the time grain. The only change that lands there is
`hideOverlap: false`, which un-hides *colliding* labels but cannot add ticks
the scale never generated โ and the reported "every 4th label" thinning comes
from the nice-interval tick algorithm, not from collision hiding. Net: the
linked issue's scenario is unchanged.
Fix: when `showAllXAxisLabels && xAxisType === AxisType.Time`, also set
the **axis-level** `interval` (value units, i.e. ms โ not `axisLabel.interval`)
to `TIMEGRAIN_TO_TIMESTAMP[resolvedTimeGrain]`. ECharts forwards that to the
scale as `userInterval` (`coord/axisNiceTicks.js:183`), which forces one tick
per grain; the map is already imported here for `minInterval`. **regression
test:** `transformProps` with `coltypes: [GenericDataType.Temporal]`, a
`time_grain_sqla`, and `xAxisLabelInterval: '0'`, asserting the forced tick
spacing on `xAxis.interval` โ not just `axisLabel.interval === 0`, which passes
today regardless.
### ๐ก Should-fix
-
**`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:2441`**
โ the fixture leaves `coltypes: []`, so `xAxisDataType` is `undefined` and
`getAxisType` falls through to `AxisType.Category` (`src/utils/series.ts:948`).
The new test therefore covers only the axis type where `interval` already
works, and asserts the option object rather than label output โ both assertions
stay green even with the temporal path broken. Add a temporal-coltype case.
-
**`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:777`**
โ same behavioral change, no test; codecov's single uncovered patch line is
this branch. Add a MixedTimeseries "All" case mirroring the Timeseries one.
### ๐ต Nits
- `superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:1137`
โ `Number('')` is `0`, so a stored or imported `xAxisLabelInterval: ''`
silently becomes "show every label, `hideOverlap` off". The control can't emit
it (`freeForm: false, clearable: false` in `src/controls.tsx:371`), but
form_data arriving via the API or an import can. Guard the empty string; the
helper test covers `null`/`undefined`/`'nope'` but not `''`.
### ๐ Praise
- `superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:1129`
โ the `hideOverlap` half is right and non-obvious: `AxisBuilder.js:604` applies
`hideOverlap` independently of the category interval, so `interval: 0` alone
would still get overridden. Deferring Bubble is also correctly reasoned โ it
sets the axis-level `interval`, a different option in value units.
<!-- enxdev-review-agent:8e9c772 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 8e9c772._
--
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]
