-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Implement period ticklabelmode on cartesian date axes #4993
Conversation
'corresponding ticks and grid lines.', | ||
'Only has an effect for axes of `type` *date*', | ||
'When set to *period*, tick labels are drawn in the middle of the period', | ||
'between ticks.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually not quite the desired behaviour: if the ticks are spaced every 2 months and the labels are Jan
and Mar
, the Jan
label should appear on Jan 15, not on Feb 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example... https://codepen.io/nicolaskruchten/pen/ZEQRBWw?editors=0010
In "instant" mode we see 4 "month" labels spaced two months apart. In "period" mode we probably should see only 3 labels, but they should remain Jan/Mar/May, and they should be positioned on Jan 15, Mar 15, May 15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a clearer codepen in the original issue
I said I was going to comment on #4911 with some more details, and I never did that, my apologies. But now that there's a PR, let me comment here in the context of your new mock. Here's the original - that has the same data and ranges as your new mock - thanks for using the same data, this makes it very clear what's going on! First, it looks like the labels moved the wrong direction - ie right now the "1950" label appears between the 1900 and 1950 tick marks, it should be between 1950 and 2000. Second, as @nicolaskruchten noted we don't want the labels in the middle of the range, but in the middle of the period DENOTED BY THAT LABEL. So the label "2000" doesn't go halfway between the ticks for 2000 and 2050, it goes halfway between the beginning of 2000 and the beginning of 2001. The important thing here is what's the smallest increment of time included in the label? If it's years, the label shifts half a year into the future. If it's months, the label shifts half a month. If it's days, the label shifts half a day. It doesn't matter if This needs to work whether the tick format is automatic, specified by Third, and this is the part I've discussed now separately with both @archmoj and @nicolaskruchten but never written down, I think What about hour precision? It's not part of our default formatting, that goes straight from days to minutes, so you won't get it by zooming in on a graph with automatic labels. You'll only see this if you explicitly specify a |
Changes made in 0e124c2 are reflected in the codepen at PR description. |
Looking much better! The day and month label offsets look perfect. The only issue I see there is that sometimes there should be an extra label at the beginning (when the tick is off-plot but the label position is on-plot), and sometimes there should NOT be a label at the end (when the tick is on-plot and the label position is off-plot). See the top row in the below image. Year tick labels have this same issue, but sometimes it looks like some of the labels get offset by half a year while others get offset by half a month? See the bottom row in the below image. When you drag this axis around sometimes a single tick will shift back and forth between half-month and half-year offsets, when it's near the end of the range. And when minutes or smaller are shown, the labels still offset. This shift should not happen at all in that case. That's the middle three rows. Finally, if you set an explicit |
778f483
to
a5e6aae
Compare
src/plots/cartesian/axes.js
Outdated
tickVals = [{ | ||
minor: false, | ||
value: axes.tickIncrement(tickVals[0].value, ax.dtick, !axrev, ax.caldendar) | ||
}].concat(tickVals); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a big deal, but tickVals.unshift({...})
is slightly more efficient here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
I also fixed reversed
range positioning and added few tests in 83f5cdd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great now! Very nice. 💃
Resolves #4911.
Demo
@plotly/plotly_js