rlei-odes commented on PR #43428:
URL: https://github.com/apache/superset/pull/43428#issuecomment-5382498805
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
`xAxis.splitLine` in only one place, the line in question. ECharts defaults
both category and time axes to `splitLine: { show: false }`
(`coord/axisDefault.js`), so writing `show: false` on a temporal axis is a
no-op rather than a broadening. And because custom `echart_options` are merged
*over* the computed options, a user who deliberately enables x-axis gridlines
still wins.
**`AxisType.Numeric` doesn't exist.** The enum is `Category | Value | Time |
Log`, so the suggested line wouldn't compile.
**Gating on a single axis type would introduce a bug.** `logAxis` is built
from the value-axis defaults, including `splitLine: { show: true }`, so a log
x-axis *does* draw gridlines. Restricting the override to `AxisType.Value`
would leave them on screen after the user unticked the control. The
unconditional form covers Value and Log without having to enumerate axis types,
and stays correct if a future axis type also defaults to showing them.
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.
--
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]
