Patch for Issue 716 #56

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@vu2srk

vu2srk commented Jul 9, 2012

  • Fixes the issue of the first tick label not displaying when time is on the X-axis
  • Previously UTC hours was set to 0 without taking timezone into account. This patch takes timezone offset into account to reset hours to 0.
  • In some cases, the resetting of the first tick value forced it to be less than the minimum data value which resulted in the first tick label not being displayed. This patch always displays the first tick label whether the first tick value is less than or greater than axis.min.
Patch for Issue 716
- Take timezone into account when setting UTC hours
- Always display first tick label

@ghost ghost assigned dnschnur Jul 10, 2012

@dnschnur

This comment has been minimized.

Show comment Hide comment
@dnschnur

dnschnur Jul 11, 2012

Owner

I think I understand the approach you're taking, but there are a few problems:

  1. Eliminating the tick.v < axis.min check doesn't work in cases where there are legitimately ticks that fall outside the axis min/max. The most common example is when using the zoom plugin, which operates on the min/max range, but there are almost certainly others.
  2. If you bring up examples/timezones.html, you'll likely see rendering issues with that first label; it extends off the left side of the canvas.
  3. In the change to jquery.flot.time.js, you're assuming that the variable d is a date-like object of the kind returned by makeUtcWrapper, and relying on the fact that it has a 'date' attribute. But if you take a look at the dateGenerator function, when opts.timezone == "browser" it instead produces a native Date object, which has no 'date' attribute.

You could solve this by having makeUtcWrapper add another proxy method for getTimezoneOffset; then you could call it directly, i.e. d.setHours(d.getTimezoneOffset() / 60); But I'm uncertain about whether subtracting the timezone offset is correct. When 'd' is produced by makeUtcWrapper, setHours is already proxied to setHoursUTC; wouldn't that take care of the offset? Timezones are always tricky, though, so please explain your thinking; I may very well be wrong.

Overall you've made a good step into one of the most complex pieces of Flot's code, but we need to find a different approach that doesn't suffer from the problems 1 & 2 above. I'll spend some more time looking at this, and will share any ideas that I come up with.

Owner

dnschnur commented Jul 11, 2012

I think I understand the approach you're taking, but there are a few problems:

  1. Eliminating the tick.v < axis.min check doesn't work in cases where there are legitimately ticks that fall outside the axis min/max. The most common example is when using the zoom plugin, which operates on the min/max range, but there are almost certainly others.
  2. If you bring up examples/timezones.html, you'll likely see rendering issues with that first label; it extends off the left side of the canvas.
  3. In the change to jquery.flot.time.js, you're assuming that the variable d is a date-like object of the kind returned by makeUtcWrapper, and relying on the fact that it has a 'date' attribute. But if you take a look at the dateGenerator function, when opts.timezone == "browser" it instead produces a native Date object, which has no 'date' attribute.

You could solve this by having makeUtcWrapper add another proxy method for getTimezoneOffset; then you could call it directly, i.e. d.setHours(d.getTimezoneOffset() / 60); But I'm uncertain about whether subtracting the timezone offset is correct. When 'd' is produced by makeUtcWrapper, setHours is already proxied to setHoursUTC; wouldn't that take care of the offset? Timezones are always tricky, though, so please explain your thinking; I may very well be wrong.

Overall you've made a good step into one of the most complex pieces of Flot's code, but we need to find a different approach that doesn't suffer from the problems 1 & 2 above. I'll spend some more time looking at this, and will share any ideas that I come up with.

@buskerone

This comment has been minimized.

Show comment Hide comment
@buskerone

buskerone Aug 22, 2012

This patch solved my problem, but know i have the problem 2 above. To solve this i augmented the placeholder width and height. to w:750 ; h:350. (the width is fundamental) The patch works very well with 6,7,8 data series but when i have 12 data series (time series) the problem 2 above appears. Thank you for this patch!

This patch solved my problem, but know i have the problem 2 above. To solve this i augmented the placeholder width and height. to w:750 ; h:350. (the width is fundamental) The patch works very well with 6,7,8 data series but when i have 12 data series (time series) the problem 2 above appears. Thank you for this patch!

This was referenced Sep 28, 2012

Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment