Uh oh!
There was an error while loading. Please reload this page.
Fix rangebreaks overlapping and tick positions - #4831
Conversation
alexcjohnson
commented
May 14, 2020
Why do both of the new baselines have an "18:27" tick right at the end? |
| var tick0 = r2l(ax.tick0); | ||
| if(ax.tickmode === 'auto' && ax.rangebreaks && ax.maskBreaks(tick0) === BADNUM) { | ||
| tick0 = moveToEndOfBreak(tick0, ax); |
There was a problem hiding this comment.
This still bothers me - I see it's only in auto mode but it seems like this will still have strange effects, especially if you do something unusual with the bounds of the break, like [8.75, 17.25]. That said I don't understand how we're getting the results we are in axes_breaks-dtick_auto of ticks at 9am and noon, ie alternating 3-hour and 5-hour gaps. If we're hitting this block then tick0 gets set to 9am, but then to get a tick at noon we'd need a 3-hour dtick, which would then put another tick at 3pm - not what we see. And I don't think we want a 3/5 alternation as an autotick result anyway, we should regularize that to consistent 4-hour spacing.
So a couple of image tests, like you have, are good. But I think we also want to see a bunch of jasmine tests where we just set up an axis and list the ticks (positions and labels) that result within one day. That should let us cover a whole lot more cases: auto-determined dtick from 1 hour to a day and everything between, and a few manually set dtick and tick0 values; each of these tested against each of the hourly rangebreaks I mentioned in #4722 (comment)
archmoj
commented
May 20, 2020
@alexcjohnson FYI - I am still working on a dev branch to address the second part of #4722 (comment). |
archmoj
commented
May 21, 2020
Update: |
| } | ||
| // break when found all types | ||
| if(n === 2) break; |
There was a problem hiding this comment.
what if the user includes two breaks of one type, THEN a break of the other type? I suppose I can theoretically imagine wanting two disjoint hour periods - a morning shift and an evening shift, with a midday break? I wouldn't worry about doing something "pretty" in this case, perhaps just set _dayHours to something like 1 or 2 so we go straight from day to hour ticks or perhaps just use the first break. But it definitely shouldn't stop us from noticing _hasDayOfWeekBreaks
And vice versa I guess... maybe someone wants to skip Sundays and Wednesdays? Again, weird, but shouldn't stop us from catching _hasHourBreaks
| case 18: return [1, 2, 3, 6, 9]; | ||
| case 20: return [1, 2, 4, 5, 10]; | ||
| case 21: return [1, 3, 7]; | ||
| case 22: return [1, 2, 11]; |
There was a problem hiding this comment.
Oh interesting... so just a precise list of factors of the rounded number of visible hours. I'm not sure we really want all of these, and we might want to add something intermediate for the larger primes so we don't jump straight from 1 day to 1 hour, even though we can't do it precisely uniformly. But this is a great starting point anyway, we can tweak later.
However, looking at the mocks changed in this PR it seems like there's a problem when dtick is not an even fraction of a day. For example in axes_breaks-candlestick2, dtick is 18 hours (how did that happen? I only see 9 here, I would have thought it would go to 24 after that? That doesn't matter though, there are lots of periods we want that don't divide 24 evenly, this is just an example), and as a result some days have 2 ticks (at 9:30 and 12:00) and other days have only one (at 9:30).
Trying to figure out a reasonable way to handle this, in conjunction with tick0 possibly being set manually. I think what we may need to say is: when you have hourly rangebreaks and dtick < 24h, the only piece of tick0 that we consider is the time portion and we start at that time on each new day, then increment by dtick until we get to the next day, at which point we reset the time to tick0 again, so you get ticks at the same hours in every day.
9b45339 to
c5cf45aComparearchmoj
commented
Jun 3, 2020
This PR is now ready for final review. |
| if(ax.tickmode === 'array') nt *= 100; | ||
| ax._roughDTick = (Math.abs(rng[1] - rng[0]) - (ax._lBreaks || 0)) / nt; |
There was a problem hiding this comment.
Looking much nicer now!
I'm just noticing that the auto dtick we get depends on whether there's a break within the visible range or not, which seems weird, and seems like exactly what this - ax._lBreaks was designed to avoid. For example (back on everyone's favorite, axes_breaks-candlestick2 😅) if I zoom in while keeping a break in range I can't get dtick less than 2 hours:
But shift the break off the edge with exactly the same scale and it snaps to 15 min:
Is there something bad that happens if this line is left as it was?
- adjust tick spacing on x axes - fixup tests
alexcjohnson
left a comment
There was a problem hiding this comment.
Beautiful! Thanks for being persistent with this issue. This is a huge improvement - there still may be some cases we want to adjust, but that's pretty minor now, most cases look really good and the pan/zoom behavior (at least with rangesliders) is quite nice. 🎉 💃
Supersedes #4734 and fixes#4722 namely the first & second parts of #4722 (comment) i.e. when
dtickis set as well asautoticks.demo: Before vs After
This PR also fixes#4879 by 5c8055b commit.
@plotly/plotly_js