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

Issue with timezones/utc and pointIntervals #3329

Closed
philfreo opened this Issue Aug 7, 2014 · 31 comments

Comments

Projects
None yet
4 participants
@philfreo

philfreo commented Aug 7, 2014

Goal: have points represent periods like week or month (longer than day) and have the appropriate tooltips like "Week of" or "January 2013" rather than just the date. I also want each point to represent the start of a week/month in local time

Problem: when trying to have periods in local time, combined with useUTC: false, the tooltips don't work.

Please carefully read through the comments in the HTML & JS here and I think you'll see the problem:
http://jsfiddle.net/philfreo/w8c3L2pd/1/

This line causes the problem: https://github.com/highslide-software/highcharts.com/blob/4af0bb07e7c84d05f8e2ff542e6d89c436eecf84/js/highcharts.src.js#L8921

@philfreo

This comment has been minimized.

philfreo commented Aug 7, 2014

The first fiddle above shows the problem with the format of the tooltips.

This new fiddle shows a slightly different problem, where the actual dates of subsequent points are off by a day:
http://jsfiddle.net/philfreo/w8c3L2pd/2/

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Aug 8, 2014

It would be fixed by changing the relevant line to this:
`(timeUnits[n] < timeUnits.day && point.key % timeUnits[n] > 0)) {``

... but it may be argued that the tooltip actually should show the full date, since the point doesn't fall exactly on midnight of the first of each month. In that case, your first chart is the one that behaves wrong.

Also, we are missing a setting to make month or year arguments to pointInterval, because those intervals are not constant. With that feature you would be able to set pointStart to midnight of the first date of a month, and all subsequent points would be on the first date of their months. I'll look into that.

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Aug 8, 2014

Here's an addition to handle months and years: http://jsfiddle.net/highcharts/mkfaw7mh/

What do you think?

@philfreo

This comment has been minimized.

philfreo commented Aug 8, 2014

Thanks for looking into this! Overall here are my requirements that don't seem possible as of the latest stable version:

  1. Have all times displayed in local time while providing the technically correct timestamp in UTC
  2. Have each period represent the local time start of a given hour, day, week, month, quarter, or year
  3. Have the tooltips show proper format for "Week of X", "Quarter starting", "January 2013", etc.

It seems like there should be two options for achieving this:

  1. Provide data with a list of x/y tuples, where the first spot is the local time start of (in UTC) a given week/month/etc. It's easy for me to generate these x values myself. Don't provide a pointStart. Only provide a pointInterval if it helps.
  2. Provide a pointStart and pointInterval with data being a numeric list (no x values in each point)

Both of these options seem to have these issues:

  1. It seems like week/month/etc. tooltips may only work if the pointStart is a midnight in UTC. This is unfair if true.
  2. Probably (as you mentioned) pointInterval needs a proper setting for week/month/quarter/year rather than just a fixed # of hours, since the interval between each month changes.

but it may be argued that the tooltip actually should show the full date, since the point doesn't fall exactly on midnight of the first of each month

Indeed I do want each point to fall on midnight of the first of each month (however midnight of local time, which I can provide myself) - it seems (as you mentioned) the problem is pointInterval can't do this currently.

Here's an addition to handle months and years: http://jsfiddle.net/highcharts/mkfaw7mh/

The top graph tooltip starts to break around April.

Also, we are missing a setting to make month or year arguments to pointInterval, because those intervals are not constant

I agree, this would be awesome. Would want options for month/quarter/year.

@philfreo

This comment has been minimized.

philfreo commented Aug 16, 2014

Summary: adding week/month/quarter options to pointInterval, and allowing points to represent non-UTC-midnight days should enable this seemingly-common case.

@philfreo

This comment has been minimized.

philfreo commented Aug 26, 2014

@highslide-software any thoughts on adding week/month/quarter options to pointInterval? Looking to purchase Highcharts for a commercial setting and this is important to us.

@wojcikstefan

This comment has been minimized.

wojcikstefan commented Aug 27, 2014

+1

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Sep 3, 2014

Summary: adding week/month/quarter options to pointInterval, and allowing points to represent non-UTC-midnight days should enable this seemingly-common case.

  1. Adding month/quarter to pointInterval (weeks not needed, they are fixed): See http://jsfiddle.net/highcharts/mkfaw7mh/ . If this implementation works for you, we will add it to the code base, though if you have control over the implementation you can always preprocess the x,y data tuples programmatically.
  2. Allow points to represent non-UTC midnights. It seems to work well already, see http://jsfiddle.net/highcharts/8yqvttd8/ .

What do you think?

@philfreo

This comment has been minimized.

philfreo commented Sep 4, 2014

See http://jsfiddle.net/highcharts/mkfaw7mh/

In the top chart, the tooltip for the first 3 points are correct, but then it breaks starting in April. (Shows the full date instead of April 2014).

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Sep 5, 2014

The switch from normal to daylight saving time threw it off by one our. I've updated the Fiddle, please try again now.

@philfreo

This comment has been minimized.

philfreo commented Sep 5, 2014

Allow points to represent non-UTC midnights. It seems to work well already, see http://jsfiddle.net/highcharts/8yqvttd8/ .

Actually it doesn't work. Your example here is not specifying non-UTC midnights. Date.parse, when given a format like yyyy-mm-dd still interprets the input date to be UTC, [according to MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse#Differences in assumed time-zone).

If you change Date.parse('2014-01-01') to Date.parse('2014-01-01T08:00:00.000Z') (which is midnight, california time) or moment('2014-01-01').valueOf() (which is midnight localtime, where you are) then it will be parsed as a non-UTC midnight, and you'll see that the tooltip no longer works for month, etc. in either of your jsfiddles.

Here's an example of all the graphs I want, and basically none of the tooltips work yet with your pointInterval changes:
http://jsfiddle.net/mkfaw7mh/2/

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Sep 8, 2014

I've worked more on this. Here's the status (will continue tomorrow).

Updated fiddle: http://jsfiddle.net/highcharts/mkfaw7mh/4/

  • Discovered, logged and fixed issue #3418, causing wrong tooltip format when point interval was like two days, two months etc. This affected your quarterly code.
  • Implemented pointIntervalUnit option. Quarters are now set by setting the pointIntervalUnit option to month, and the pointInterval to 3. This opens up for working with intervals like 6 months, 10 years (decades), centuries etc.
  • Fixed your hour and day samples. Remember JavaScript dates are milliseconds, not seconds since 1970.

Now the next step will be fixing the actual bug so that in non-UTC charts, the tooltip shows the correct time format.

@TorsteinHonsi TorsteinHonsi added Bug and removed Undecided labels Sep 8, 2014

@philfreo

This comment has been minimized.

philfreo commented Sep 8, 2014

Cool, thanks for the update!

My only comment is regarding pointIntervalUnit – it might be nice if there were special tooltips for quarter (e.g. '2014Q1' or 'Quarter starting January 2014') vs. only having month ('January 2014') tooltips.

TorsteinHonsi added a commit that referenced this issue Sep 9, 2014

Implemented algorithm for setting precise tooltip xDateFormat when ne…
…eded, but general format like year, month, day etc. when the time falls on midnight and the closest point range dictates it. Related to #3329.
@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Sep 9, 2014

I think I have this sorted out now.

Updated fiddle: http://jsfiddle.net/highcharts/mkfaw7mh/5/

Note that the weekly tooltip falls back to days because it expects the week to start at the xAxis.startOfWeek configuration (Monday by default).

@philfreo

This comment has been minimized.

philfreo commented Sep 9, 2014

Could you show an example of how one could configure/override it to detect week intervals, regardless of what day the point starts on, so that the tooltip could say, e.g., "Week starting January 1, 2014". (Would also like to do something similar for quarters).

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Sep 17, 2014

@philfreo

This comment has been minimized.

philfreo commented Jan 22, 2015

(Sorry for the delay - just digging back into this again)

If I combine the last 2 jsfiddles to test all the various intervals in one place...

http://jsfiddle.net/krourcnt/

a) it still doesn't work properly (e.g. the April tooltip in the By Month graph improperly shows a Day/Time)
b) it seems like I shouldn't need so much custom code (shouldn't you allow pointInterval month/year in core Highcarts?)

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Jan 23, 2015

The pointIntervalUnit option is now added to the 4.1 candidate. See http://jsfiddle.net/highcharts/krourcnt/1/.

a) This works with the latest branch.
b) The tooltip header formatter for quarters and weeks must still be added programmatically.

@philfreo

This comment has been minimized.

philfreo commented Jan 26, 2015

Thanks. And what is the explanation for needing the custom tooltip formatter for weeks/quarter? Couldn't you argue that this should be the default and that it should use tooltip.dateTimeLabelFormats.week and tooltip.dateTimeLabelFormats.quarter ?

@philfreo

This comment has been minimized.

philfreo commented Jan 30, 2015

Any word on this?

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Feb 5, 2015

Yes possibly... Except if you are acutally assigning data for January, April etc...

@philfreo

This comment has been minimized.

philfreo commented Feb 9, 2015

Yes possibly... Except if you are acutally assigning data for January, April etc...

That sounds like a much less common scenario than people using pointIntervalUnit === 'month' and pointInterval === 3 for quarters. The default tooltip formatting should probably cover the most common use case, no? The same applies to a pointIntervalUnit of week.

Typically if people just want monthly data they wouldn't say pointInterval === 3, right?

@eskimoblood

This comment has been minimized.

eskimoblood commented Mar 31, 2016

What is the state of the day light saving time offset issue. When I test it with code from one of the latest fiddle in threat i seems to be fixed, the latest version from the CDN still add the offset. Example: http://jsfiddle.net/2s0grfeb/

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Mar 31, 2016

This is officially released. In your fiddle, I can't see a difference between using the CDN script or the commented-out one from GitHub...

@eskimoblood

This comment has been minimized.

eskimoblood commented Mar 31, 2016

This is what I get for the CDN version:
google chrome - github com - - screen shot 31 mar 2016 14 41 30

note the 01:00 after the date, this does not happen in the other version.

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Mar 31, 2016

Ok, it actually shows the correct value because you're starting in winter time and add 24 hours repeatedly. What you probably want to do is to set the data points on local midnight instead of using pointInterval.

@eskimoblood

This comment has been minimized.

eskimoblood commented Mar 31, 2016

Ok, so there is no way to prevent this when using pointInterval? Any plans to add pointInterval: 'day', like add(1, 'day') in momentJS, cause this would be the expected behaviour.

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Mar 31, 2016

No, currently pointIntervalUnit only supports month and year.

Any plans to add pointInterval: 'day', like add(1, 'day') in momentJS, cause this would be the expected behaviour.

That would be pointIntervalUnit: 'day'. And I agree, it would be useful. I'm looking at it right away.

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Mar 31, 2016

Done. See http://jsfiddle.net/highcharts/2s0grfeb/1/, and compare to the other series that has a hard 24h interval. Note that this demo is tested on a browser in Central European Time, the DST crossover being March 28.

A related problem now appears with the tooltip showing 00:00 for every date. See #5128.

@eskimoblood

This comment has been minimized.

eskimoblood commented Mar 31, 2016

Thanks, thats was fast. When is this feature planed to be released?

@TorsteinHonsi

This comment has been minimized.

Collaborator

TorsteinHonsi commented Apr 1, 2016

You're welcome! It will be released within a few weeks.

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