Uh oh!
There was an error while loading. Please reload this page.
handle various time formats in ticklabelmode period - #5065
Conversation
| else if( | ||
| _has('%A') || // full weekday name | ||
| _has('%a') || // abbreviated weekday name | ||
| _has('%U') || // Sunday-based week of the year as a decimal number [00,53] |
There was a problem hiding this comment.
where in the code do you differentiate between the start point of %U and %W (Sunday vs Monday) ?
There was a problem hiding this comment.
No. This is just the fall back to set the distance between labels.
There was a problem hiding this comment.
OK, but then how can we differentiate between sunday- and monday-starting weeks? via tick0 ?
There was a problem hiding this comment.
Good point - it's a bit different from the point of this PR, but here's where the automatic tick0 gets set to Sundays:
plotly.js/src/plots/cartesian/axes.js
Lines 876 to 879 in cfc8c2c
I guess with a
tickformat including %W we should add a day to the result there.There was a problem hiding this comment.
I believe we should, yes :)
There was a problem hiding this comment.
Also %V starts on Mondays.
Side note: Am I missing something or are %u (new week on Monday) and %w (new week on Sunday) backward compared to %U (new week on Sunday) and %W (new week on Monday)? I mean, not an error here, nor in D3, this dates back to strftime from C so has probably been around for 40 years...
alexcjohnson
left a comment
There was a problem hiding this comment.
@archmoj functionally this looks good. I feel like perf and compactness may improve with a single regex test, eg /%[fLQsSMHIpX]/.test(ax.tickformat) instead of repeated indexOf calls. Not a big deal though. Just need a couple of jasmine tests, and your call whether to add the tick0 Sunday/Monday fix here or in a separate PR.
nicolaskruchten
commented
Aug 12, 2020
Here's a pen based on this bundle: https://codepen.io/nicolaskruchten/pen/rNeOgXj?editors=0010 (no rangebreaks) ... the label doesn't appear in the middle of the week unfortunately |
| ) definedDelta = ONEDAY; | ||
| else if( | ||
| _has('%A') || // full weekday name | ||
| _has('%a') || // abbreviated weekday name |
There was a problem hiding this comment.
Don't weekdays go in the ONEDAY bucket?
| afterEach(destroyGraphDiv); | ||
| function _assert(msg, exp) { | ||
| var labelPositions = gd._fullLayout.xaxis._vals.map(function(d) { return d.periodX; }); |
There was a problem hiding this comment.
Very nice tests! But can we use _fullLayout.xaxis.c2d(d.periodX) to get these back into date string form - eg 1562079600000 would give '2019-07-02 15:00', and it's not clear to me how we got that precise value, but at least it has a recognizable meaning.
There was a problem hiding this comment.
Also perhaps we can test the text that's stored in xaxis._vals[].text so it's obvious what labels we're showing at what datetime values?
There was a problem hiding this comment.
Very nice tests! But can we use
_fullLayout.xaxis.c2d(d.periodX)to get these back into date string form - eg1562079600000would give'2019-07-02 15:00', and it's not clear to me how we got that precise value, but at least it has a recognizable meaning.
Good call. Done in f0f078a.
There was a problem hiding this comment.
Also perhaps we can test the text that's stored in
xaxis._vals[].textso it's obvious what labels we're showing at what datetime values?
Done in 90ee834.
| }) | ||
| .then(function() { |
There was a problem hiding this comment.
| }) | |
| .then(function(){ |
Not a big deal, but here (and the other new tests) these assertions can be done together.
alexcjohnson
left a comment
There was a problem hiding this comment.
Beauty! Thanks for the test changes, much easier to see what's really going on 💃
Follow up of implementing #4911 by #4993 and considering adding new time-formats added in #5026.
@plotly/plotly_js