Implement options to draw minor ticks and grid lines in Cartesian subplots
#6166
Conversation
| minor: { | ||
| tickmode: tickmode, | ||
| nticks: nticks, | ||
| tick0: tick0, |
There was a problem hiding this comment.
Do we actually want to support tick0 here? Strange things could happen, which may be fine if (a) axis.minor.tick0 inherits from axis.tick0 (even if axis.tick0 is set during autotick as in the case of week ticks), and (b) we test out the relevant cases and ensure we like the results.
I guess this is the main thing we want to test in jasmine, right? First the standard cases, where you provide major tick settings and various combinations of minor nticks, dtick, and tick0 and verify that the correct minor ticks are generated.
Even without explicit tick0 there are questions about what to do when dtick is not an integer multiple of minor.dtick - do we keep the minor tick spacing consistent, or do we start fresh from every major tick? Presumably the former, even though that means two minor ticks will straddle one major. But I could certainly see wanting this for example to put week minor ticks with month major ticks.
|
Minor drawing issues: 2 - Y minor gridlines are on top of X major, can we put them behind? |
src/plots/cartesian/axes.js
Outdated
| majorDtick.charAt(0) === 'M' | ||
| ) { | ||
| if(majorDtick === 'M24') { | ||
| ax.dtick = 'M12'; |
There was a problem hiding this comment.
@archmoj I love the new auto-minor algorithm! When you simply turn on minor ticks it's perfect as far as I can see 🏆 😍
That said I think these date cases here are still a little too hard-coded if you stray from full auto mode. For example if I set minor.nticks=12 or larger I'd expect year ticks to give one-month minor, 2-year ticks to give 2-month minor, etc. If I do that right now, 2-year ticks get 1-year minor, but when you zoom out to 5-year ticks you DROP to (the expected) 6-month minor ticks.
src/plots/cartesian/axes.js
Outdated
| } | ||
| } | ||
| } else { | ||
| dist /= nt || 7; |
There was a problem hiding this comment.
Small (minor? 😏 ) point: on linear axes, if I want minor.dtick===dtick/10 I don't get this with minor.nticks: 10, I have to go to 11
Also, I was expecting if I provided major dtick and tick0 plus minor.dtick, that minor.tick0 would be inherited from tick0 - but it seems unless I explicitly provide it to match, minor.tick0 reverts to the default.
Definitely better, and we can leave all of this (including the 0.5px outside gap ^^) as a bug to come back to later if you prefer, since it was already there with major ticks only. But whenever we do address it, it seems like there's still some condition where the inside ticks don't quite reach as far as they should. See
|
FYI - I opened #6173 which would be addressed in a separate PR to avoid several baseline changes unrelated to this feature PR. |
| var positions = | ||
| ax._vals | ||
| .filter(function(d) { return d.minor; }) | ||
| .map(function(d) { return d.x; }); |
There was a problem hiding this comment.
nonblocking: the date and log tests would be more readable if the values were in data format, ie I think ax.l2d(d.x)
looks confusing to have minor ticks 2,5,10,20,50, so drop to just the orders of magnitude. D1 is fine though, if you ask for that many minor ticks!
There was a problem hiding this comment.
I think we're finally there! Thanks for all the hard work @archmoj, this one was definitely more involved than it originally appeared!
💃 🕺 🕺 🕺 💃 🕺 🕺 🕺 💃 🕺 🕺 🕺 💃
|
Great feature. Congrats @archmoj ! |
Thanks very much @etpinard. |
…or ticks and grid lines on cartesian axis types (plotly/plotly.js#6166)
Resolves #903.
@plotly/plotly_js