Skip to content

rangebreaks sometimes draws the gridline on the wrong point #4692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nicolaskruchten opened this issue Mar 26, 2020 · 9 comments · Fixed by #4696
Closed

rangebreaks sometimes draws the gridline on the wrong point #4692

nicolaskruchten opened this issue Mar 26, 2020 · 9 comments · Fixed by #4696
Assignees
Labels
bug something broken
Milestone

Comments

@nicolaskruchten
Copy link
Contributor

See Jan 3 here: https://codepen.io/nicolaskruchten/pen/oNXQmLR?editors=1010

@alexcjohnson
Copy link
Collaborator

It's not drawing the gridline on the wrong point; it's drawing a gridline inside the break, but due to the way the breaks work (removing from the first instant of Saturday until the last instant of Sunday), the break is at Monday.

Simplest fix I can think of here is to shift the auto tick0 to a Monday when we remove the weekend - more precisely, take the first day of week break and shift tick0 to the day in its bounds[1]

That happens here:

ax.tick0 = (ax.type === 'date') ? '2000-01-01' : 0;

but mostly here (you can see we explicitly chose Sunday):

// get week ticks on sunday
// this will also move the base tick off 2000-01-01 if dtick is
// 2 or 3 days... but that's a weird enough case that we'll ignore it.
ax.tick0 = Lib.dateTick0(ax.calendar, true);

@alexcjohnson
Copy link
Collaborator

Oh actually ignore line 542, I'm not sure when that happens but I suspect it's largely for pseudo-axis fallbacks.

@archmoj
Copy link
Contributor

archmoj commented Mar 26, 2020

Removed my previous comment which was not correct.

@archmoj archmoj added the bug something broken label Mar 26, 2020
@nicolaskruchten
Copy link
Contributor Author

@alexcjohnson are you proposing a weekend-specific fix or do you think this is a generalizable approach?

@nicolaskruchten
Copy link
Contributor Author

Also, we should make sure this works well with bars and tickson = "boundaries"!

@alexcjohnson
Copy link
Collaborator

I was suggesting weekend-specific. I feel like any attempt to generalize beyond that will create other confusion... maybe not, maybe if you also remove a specific Monday we could make the tick say Tuesday somehow. That seems tougher to implement though. We could try and just use the end of the break as the tick value (ie reverting the special logic I had @archmoj figure out to position the tick at the end of the break but label it with the label it would have gotten in the absence of breaks) and see if the label comes out right. I just worry about edge cases like if you remove until 1pm Monday, will the tick say Tuesday even though there are 11 hours of Monday left after the tick? The reason for my concern is that we aggressively set the tick label rounding based on dtick and tick0, so if we start moving the ticks off of even multiples of dtick it's unclear what will happen.

Also, we should make sure this works well with bars and tickson = "boundaries"!

in that future where breaks apply to category axes, or tickson applies to dates 😁

@nicolaskruchten
Copy link
Contributor Author

in that future where breaks apply to category axes, or tickson applies to dates 😁

Oh right :) Phew!

@nicolaskruchten
Copy link
Contributor Author

use the end of the break as the tick value

This seems like a good idea to me actually, and in fact will be more forward-compatible with a world where we actually render a gap where the break is, right? The break would come right before the gridline/ticklabel in that case. Using some value in the break is not going to work well in that case.

@alexcjohnson
Copy link
Collaborator

OK, we can try it. Maybe it'll just generally work out? It'll certainly be simpler code, with apologies to @archmoj for the extra complexity (specifically the _realV stuff) that will just need to be reverted to do it this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants