rusackas commented on code in PR #43729:
URL: https://github.com/apache/superset/pull/43729#discussion_r4078457979
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -2877,6 +2877,77 @@ test('boundary label alignment is dropped when the
orientation moves the time ax
expect(horizontal.axisLabel.alignMaxLabel).toBeUndefined();
// The boundary labels themselves stay forced in both orientations.
+
+describe('xAxisLabelInterval string "0" is converted to number 0', () => {
Review Comment:
Confirmed fixed, that block is at module scope now (landed in 1052764c6).
Ran the full suite locally across all three touched test files (223 tests) to
make sure nothing's silently not registering anymore.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1254,7 +1254,7 @@ export default function transformProps(
showMaxLabel,
xAxisType,
xAxisLabelRotation,
- xAxisLabelInterval,
+ xAxisLabelInterval === '0' ? 0 : xAxisLabelInterval,
Review Comment:
This part's already covered, hideOverlap goes off whenever showAllLabels is
true regardless of whether ticks are pinned, so numeric 0 isn't getting thinned
there.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1254,7 +1254,7 @@ export default function transformProps(
showMaxLabel,
xAxisType,
xAxisLabelRotation,
- xAxisLabelInterval,
+ xAxisLabelInterval === '0' ? 0 : xAxisLabelInterval,
Review Comment:
hideOverlap disabling is axis-type-agnostic, so this technically already
covered time axes, but you're right the test only checked the option value and
never exercised it. Added a test that renders the real axisLabel.formatter
across 30 daily points close enough to collide and asserts none come back blank.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -1114,18 +1114,31 @@ export function getTemporalAxisTickConfig(
const cappedTickValues = temporalTickValues
? capTickMarks(temporalTickValues)
: undefined;
- const labelCustomValues = zoomable ? temporalTickValues : cappedTickValues;
+ // When the user picks "All" (interval === 0), they want every label shown.
+ // Disable hideOverlap so ECharts never drops a label, and pin customValues
+ // to the full tick set so each label lands on a real gridline.
+ const showAllLabels = xAxisLabelInterval === 0;
+ // On a zoomable axis the full set is already used; for "All" we also bypass
+ // the cap so every tick gets a label rather than the 60-mark subset.
+ const labelCustomValues = zoomable || showAllLabels
+ ? temporalTickValues
+ : cappedTickValues;
+
return {
axisLabel: {
// Pinned ticks label every bucket, which does crowd, so thinning
- // always wins there.
+ // wins there unless the user asked for every label.
hideOverlap:
- !!temporalTickValues ||
- (showMaxLabel
- ? false
- : !(xAxisType === AxisType.Time && xAxisLabelRotation !== 0)),
+ !showAllLabels &&
Review Comment:
Good catch. Added a showAllLabels bypass to createSpacedXAxisFormatter
itself so this path skips the spacing check too (the identical-label dedup
still applies). Covered with a direct unit test on the formatter plus the
daily-points transformProps test.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -1114,18 +1114,31 @@ export function getTemporalAxisTickConfig(
const cappedTickValues = temporalTickValues
? capTickMarks(temporalTickValues)
: undefined;
- const labelCustomValues = zoomable ? temporalTickValues : cappedTickValues;
+ // When the user picks "All" (interval === 0), they want every label shown.
+ // Disable hideOverlap so ECharts never drops a label, and pin customValues
+ // to the full tick set so each label lands on a real gridline.
+ const showAllLabels = xAxisLabelInterval === 0;
+ // On a zoomable axis the full set is already used; for "All" we also bypass
+ // the cap so every tick gets a label rather than the 60-mark subset.
+ const labelCustomValues = zoomable || showAllLabels
Review Comment:
Agreed, uncapped axisTick specifically for the "All" case. Left the
zoomable-axis cap alone since that one's a deliberate separate tradeoff
(already covered by its own test). Added a 261-bucket test asserting axisTick
matches axisLabel exactly when interval is 0.
--
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]
