feat(plugin-chart-echarts): add gridline and axis tick controls - #43428
feat(plugin-chart-echarts): add gridline and axis tick controls#43428rlei-odes wants to merge 1 commit into
Conversation
The minor gridlines and minor ticks are exposed as controls, but their
main counterparts are hardcoded in the transform, one line apart:
minorSplitLine: { show: isSmallChart ? false : minorSplitLine },
splitLine: { show: !isSmallChart },
axisTick: { show: !isSmallChart },
Adds the two matching checkboxes, on every chart that already carries the
minor pair. Both default to on and only ever subtract: where an axis had
no such key, the enabled state leaves ECharts' own default in place
rather than forcing the option on, so existing charts render unchanged.
dosubot
Bot
added
change:frontend
Code Review Agent Run #48e5d8Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| }, | ||
| minorTick: { show: minorTicks }, | ||
| axisTick: { show: axisTicks ? 'auto' : false }, | ||
| ...(gridlines ? {} : { splitLine: { show: false } }), |
There was a problem hiding this comment.
Suggestion: When gridlines is false, this unconditionally adds splitLine.show = false to the x-axis, including temporal axes. The control is intended to control the value-axis gridlines and only suppress x-axis gridlines when the x-axis is numeric; this currently overrides any existing temporal x-axis split-line configuration and makes the behavior broader than the control's documented scope. Restrict the x-axis override to numeric x-axis types. [api mismatch]
Severity Level: Minor 🧹
- ⚠️ Temporal x-axis split-line configuration is suppressed unexpectedly.
- ⚠️ Gridlines control exceeds its documented numeric-axis scope.
- ⚠️ Returned ECharts options differ for temporal charts.
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:** 1284:1284
**Comment:**
*Api Mismatch: When `gridlines` is false, this unconditionally adds `splitLine.show = false` to the x-axis, including temporal axes. The control is intended to control the value-axis gridlines and only suppress x-axis gridlines when the x-axis is numeric; this currently overrides any existing temporal x-axis split-line configuration and makes the behavior broader than the control's documented scope. Restrict the x-axis override to numeric x-axis types.
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. The current implementation in Here is the corrected implementation for minorTick: { show: minorTicks },
axisTick: { show: axisTicks ? 'auto' : false },
...(gridlines || xAxisType !== AxisType.Numeric ? {} : { splitLine: { show: false } }),
minInterval:
xAxisType === AxisType.Time && resolvedTimeGrain && !forceMaxIntervalI have checked the other comments on this PR, and there are no additional review comments to address. Would you like me to perform any further analysis? superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts |
|
Thanks — I looked into this and I'd like to keep the current form, for three reasons. Nothing is being overridden on temporal axes. The transform sets
Gating on a single axis type would introduce a bug. The control only ever subtracts — ticked writes nothing, so no axis can gain gridlines it doesn't already have. You did catch a real inaccuracy in the PR description, though: it said the x-axis is only affected "when that axis is numeric", which is narrower than what the code does. I've corrected that wording. |
1 participant
SUMMARY
The ECharts timeseries family exposes Minor ticks and Minor Split Line
as controls, but the main gridlines and main axis ticks are hardcoded in the
transform, on the lines immediately around them:
So the faint gridlines between the labelled ticks can be turned off, while the
main ones they subdivide cannot. That asymmetry reads as accidental rather than
deliberate.
This adds the two matching checkboxes, Gridlines and Axis ticks, to the
Customize tab of every chart that already carries the minor pair: Bar, Line,
Area, Step, Scatter, Smooth Line and Mixed.
Each control covers exactly the axes its minor counterpart already covers:
minorSplitLineminorTicksIn practice that means the value axis, which is why it mirrors
minorSplitLine.ECharts defaults category and time axes to
splitLine: { show: false }andvalue and log axes to
show: true, so unticking writesshow: falseon bothaxes without enumerating axis types: it hides gridlines where they exist and is
a no-op where they do not. Ticking never writes the key at all, so no axis
gains gridlines it does not already have.
Three details worth flagging for review:
one. The transform destructures from
{ ...DEFAULT_FORM_DATA, ...formData },so charts saved before this change pick the defaults up too. Where an axis had
no
axisTickkey at all, the ticked state writesshow: 'auto'rather thanshow: true—'auto'is ECharts' own default, and it resolves to hiddenon a banded category axis. Forcing
truewould have added tick marks thatbar charts do not currently draw.
gridlines and ticks below
compactChartHeight, and the new controls arewritten as
isSmallChart ? false : gridlines— matching the line above them —so a ticked box cannot put gridlines back on a chart too small to carry them.
splitLine: { show: false }unconditionally, as it does today, so the two grids can never double up.
The control governs the primary axis only.
Neither of its axes overrides
axisTick, and ECharts''auto'resolves tohidden whenever the other axis is a category or time scale — which covers most
Mixed charts. The checkbox is subtractive, so with no ticks drawn there is
nothing for it to hide. Making it meaningful there would mean adding ticks
by default, which this PR deliberately does not do. Happy to drop the control
from the Mixed panel instead if reviewers prefer.
Motivation: a sparse, low-gridline chart currently requires hand-written JSON in
Customize → ECharts Options. Background:
#43426
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before — gridlines and axis ticks as they render today.
After — the same chart with both unticked.
The two new controls, on the Customize tab beside the minor pair they
mirror.
TESTING INSTRUCTIONS
Unit tests:
npm run test -- plugins/plugin-chart-echarts/test/Timeseries/transformProps plugins/plugin-chart-echarts/test/MixedTimeseries/transformPropsManually, on any Bar or Mixed chart:
renders exactly as before — this is the regression that matters most.
disappear; the axis labels and ticks stay.
gridlines still follow the value axis, which is now the horizontal one.
hidden regardless of the checkboxes, as they do today.
Charts verified by hand
Step and Smooth Line share
Timeseries/transformProps.tswith Bar, Line, Areaand Scatter, with no branching between them, so the four checked charts cover
the same code. Histogram, Waterfall and Box Plot are untouched: they carry
neither
minorTicksnorminorSplitLine, and this PR only adds controls wherethe minor pair already exists.
Two things noticed while testing that are not caused by this PR, recorded in
case they are news:
control works on Bar, Line and Mixed. Those charts run the identical
transform, and this PR does not touch
minorTickin any file, so the causelies in how ECharts computes minor tick positions for that axis extent
(
getMinorTicksCoordsreturns an empty list and the builder bails).is ECharts' own defaulting:
minorTickinherits its stroke fromaxisTick,falling back to the axis line colour, whereas
splitLinehas its own themetoken.
ADDITIONAL INFORMATION