Repository navigation
fix(echarts): honor "All" for the x-axis label interval - #43193
bikash-barnwal wants to merge 2 commits into
Conversation
dosubot
Bot
added
change:frontend
Code Review Agent Run #118e34Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43193 +/- ##
=======================================
Coverage 66.64% 66.65%
=======================================
Files 2873 2873
Lines 163625 163636 +11
Branches 37767 37771 +4
=======================================
+ Hits 109056 109066 +10
- Misses 52440 52441 +1
Partials 2129 2129
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The X Axis Label Interval control offers Auto and All, storing All as the string '0'. ECharts' `axisLabel.interval` only understands 'auto' or a number, so the string was ignored and picking All changed nothing. Normalize the control value to what ECharts accepts, falling back to 'auto' for anything unparseable rather than handing the axis a bad value. Asking for every label also has to win over `hideOverlap`, which is on for non-rotated axes and would otherwise hide the very labels the setting exists to show, so the helper reports that case and both Timeseries and MixedTimeseries disable hideOverlap for it. Bubble passes the same control value to `xAxis.interval` -- a different option with different units -- which is left alone here since changing it would alter tick spacing rather than label density. Fixes apache#36325 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
812af79 to
8e9c772
Compare
Code Review Agent Run #b00eedActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Pull request overview
Fixes an ECharts Timeseries/MixedTimeseries bug where selecting X Axis Label Interval = All (stored as the string '0') was ignored by ECharts, and where hideOverlap could still suppress labels even when “All” was requested.
Changes:
- Added
getAxisLabelIntervalhelper to coerce the control value into ECharts-compatibleaxisLabel.intervalvalues ('auto'or a number) and to flag the “show all labels” case. - Updated Timeseries and MixedTimeseries
transformPropsto use the normalized interval and to disablehideOverlapwhen “All” is selected. - Added unit test coverage for the helper and regression tests to ensure
interval === 0andhideOverlap === falsefor “All”, and that the default remains'auto'.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts | Adds getAxisLabelInterval to normalize the x-axis label interval control value for ECharts and detect “show all labels”. |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | Uses the helper to set axisLabel.interval correctly and disables hideOverlap when “All” is selected. |
| superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts | Same normalization + hideOverlap override as Timeseries for consistent behavior. |
| superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts | Adds unit tests for getAxisLabelInterval normalization and fallbacks. |
| superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts | Adds regression tests asserting “All” results in interval: 0 and hideOverlap: false, and that the default is 'auto'. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
EnxDev's Review Agent — #43193 · HEAD 8e9c772comment — the category-axis half is correct and well-argued, but on a temporal x-axis (the case in #36325) ECharts still ignores 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
|
|
@bikash-barnwal the category-axis part of this looks right, but I don't think it closes #36325. ECharts only reads The new regression test in Also needs a rebase, this is showing conflicts with |
github-actions
Bot
added
the
requires:rebase
ECharts only ever reads axisLabel.interval for category axes (axisTickLabelBuilder routes to makeCategoryLabels there; a time axis goes through makeRealNumberLabels and never touches it). Superset sets xAxis.type to Time whenever the x-axis column is temporal, so the scenario apache#36325 actually reports -- a day/week grain on a temporal column -- went through the code path the previous commit didn't change; the regression test used the base fixture's coltypes: [], exercising only the already-working category path. On a time axis, tick density comes from minInterval/maxInterval instead. minInterval alone only floors the spacing -- ECharts can still pick a wider nice interval to fit the available width -- so pin both bounds to the resolved time grain when "All" is selected, forcing one tick (and label) per data point. "All" wins over forceMaxInterval's own opposite request. Without a resolved grain there's nothing to pin to, so that case falls back to the pre-existing behavior. Added a regression test with a temporal coltype and a Day grain, and a fallback-behavior test for the no-resolved-grain case. Applied the same fix to MixedTimeseries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You're right, thanks for tracing that through On a time axis, tick density comes from Added a regression test using a temporal coltype + Day grain (mirroring the tooltip-time-grain tests' fixture pattern) asserting Re: the rebase — I hit a shallow-clone issue attempting it locally (this fork's clone doesn't share full history with |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const timeGrainIntervalMs = resolvedTimeGrain | ||
| ? (TIMEGRAIN_TO_TIMESTAMP[ | ||
| resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP | ||
| ] ?? undefined) | ||
| : undefined; |
There was a problem hiding this comment.
Suggestion: Calendar grains use fixed durations, so pinning a month, quarter, or year can create ticks on drifting dates and labels for missing time periods. [logic error]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 1221:1225
**Comment:**
*Logic Error: Calendar grains use fixed durations, so pinning a month, quarter, or year can create ticks on drifting dates and labels for missing time periods.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. Calendar grains (like months or quarters) have variable durations, so using a fixed duration (like a constant number of milliseconds) to pin ticks can lead to misaligned labels or missing time periods when the underlying data does not perfectly align with that fixed duration. To resolve this, you should avoid pinning the interval to a fixed duration for calendar-based grains. Instead, you can rely on ECharts' built-in time axis handling, which is designed to calculate appropriate intervals for calendar units automatically. If you need to ensure all labels are shown, you should handle the interval logic by checking if the grain is a calendar type and adjusting the Would you like me to fetch all other comments on this PR to validate them and implement a minimal fix for the rest? superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts |
Code Review Agent Run #0ffc2cActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Thanks for tracing the category/time-axis split; normalizing axisLabel.interval is a sensible part of the fix. I found two temporal cases that still need another pass before this closes #36325: several supported grains bypass the new path, and pinning a continuous scale synthesizes ticks across gaps. I left concrete reproductions inline. The two added suites pass locally (180 tests), although they only inspect the generated options rather than the rendered ticks. The branch also conflicts with master; the recent time-axis work in #43339 and #43669 has helpers that may make an actual-timestamp approach easier.
| ] ?? undefined) | ||
| : undefined; | ||
| const showAllOnTimeAxis = | ||
| showAllXAxisLabels && xAxisType === AxisType.Time && !!timeGrainIntervalMs; |
There was a problem hiding this 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?
| ] | ||
| xAxisType === AxisType.Time && resolvedTimeGrain | ||
| ? showAllOnTimeAxis || forceMaxInterval | ||
| ? timeGrainIntervalMs |
There was a problem hiding this 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?
|
@EnxDev's points on this seem right to me. TIMEGRAIN_TO_TIMESTAMP only covers hour/day/month/quarter/year, so weekly grains fall through and "All" still won't work for the Week case the original issue reports. The sparse-data tick explosion (thousands of ticks for a couple of far-apart points) sounds like a real problem too. Also still conflicting with |
|
Still no update since my last check-in. |
SUMMARY
Setting X Axis Label Interval to All did nothing — charts kept showing every nth label.
The control is a
SelectControlwhose choices are['auto', 'Auto']and['0', 'All'], so All is stored as the string'0'. ECharts'axisLabel.intervalonly understands'auto'or a number; a string is not a valid value, so it was ignored and the axis fell back to its default label thinning.There is a second half to it. Even with a numeric
0,hideOverlapis enabled for non-rotated axes:and
hideOverlapdrops labels that collide — exactly the labels All exists to show. So fixing only the type coercion would still not honor the setting.This adds a small
getAxisLabelIntervalhelper that normalizes the control value to what ECharts accepts (falling back to'auto'for anything unparseable, rather than handing the axis a bad value) and reports whether the user asked for every label. Timeseries and MixedTimeseries use it to setintervaland to disablehideOverlapin that one case.Left alone deliberately: Bubble passes the same control value to
xAxis.intervalrather thanaxisLabel.interval— a different option, measured in value units rather than label counts. Changing it would alter tick spacing, not label density, so it is out of scope here and worth a separate look.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: with All selected, an Area/Time-series chart still renders roughly every 4th label.
After: every category label renders.
TESTING INSTRUCTIONS
775 tests across 69 suites pass. New cases: a unit test for the helper (string
'0','auto',undefined,null, a nonsense value, and an explicit2), and a Timeseries test assertinginterval === 0withhideOverlap === false. Two of the three fail on unpatched source; the third pins the'auto'default so the new branch cannot regress it.Manually: any Time-series or Area chart → Customize → X Axis Label Interval → All, and confirm every label renders.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code