Fix period positioned hover to work in different time zones as well as on grouped bars #5864
Conversation
| } | ||
| } | ||
|
|
||
| val = ax.d2c(val); |
There was a problem hiding this comment.
@archmoj Can you explain why the previous code was timezone-dependent, and how this fixes it?
There was a problem hiding this comment.
The previous logic used start and end of period directly which appears to depend on the time zone.
But now we only use the difference between two which should not be impacted by time zone.
We will keep #5861 open until we could further investigate other time zone problems.
There was a problem hiding this comment.
Ah ok, I think I understand it now. The previous one had the advantage of front-loading more of the calculation, so let's try and get back toward that. I think what's going on is when you pull out the period with:
var period = winningPoint[axLetter + 'Period'];This is already in calcdata format - ie a number - so when we run ax.d2c on it we get the legacy convert-from-local-to-UTC behavior. But val is in data format - ie a string - so it needs ax.d2c. So the right solution is to just change the one line:
val = ax.d2c(period !== undefined ? period : val);To:
val = period !== undefined ? period : ax.d2c(val);There was a problem hiding this comment.
Thanks for the hint. There was actually a problem in new code which is fixed now.
Unfortunately trying val = period !== undefined ? period : ax.d2c(val); didn't pass the tests.
However xStart and xEnd are coming from the calc step so the changes should be fast.
There was a problem hiding this comment.
Hmm OK - well, this seems to work so let's go with it. Thanks for investigating!
src/components/fx/hover.js
Outdated
| } | ||
|
|
||
| var cd0 = winningPoint.cd[winningPoint.index]; | ||
| if(cd0 && cd0.t && cd0.t.posLetter === ax._id) { | ||
| if(d && d.t && d.t.posLetter === ax._id) { |
There was a problem hiding this comment.
This code has been around for ages but I'm just noticing it now and it confused the heck out of me: normally the t object only exists on the first entry in cd (that's why the var we extracted it from was called cd0) because it contains extra calculated info that applies to the whole trace. And at the calc step that's true of box and violin too. But then during the plot step it appears these traces add t to all cd entries so that if you draw points, the one box the points are for will look like a whole trace to the reused scatter code with which we're drawing the points.
So yes, this works... but only due to a quirk of our drawing code and only for box and violin, so it feels fragile. Nonblocking, but when we need t we should really get it from cd[0].
There was a problem hiding this comment.
What you suggesting here is renaming d back to cd0? Or I should simply add a comment above cd0.d?
There was a problem hiding this comment.
I'm saying instead of assuming box & violin will have t in every element of cd (d is cd[winningPoint.index] and winningPoint.index need not be zero) we should be using cd[0].t.
There was a problem hiding this comment.
Addressed in 3e52b94
archmoj
changed the title
| - run: | ||
| name: Run hover_label test in America/Toronto timezone | ||
| environment: | ||
| TZ: "America/Toronto" |
There was a problem hiding this comment.
Do we need all these time zones to be tested?
There was a problem hiding this comment.
It's pretty quick, might as well keep them all for now.
Fixes #5822 (comment) | cc: #5861
Also closes #5841.
@plotly/plotly_js