Skip to content
This repository was archived by the owner on Jul 29, 2019. It is now read-only.

Fix axis not displayed after daylight saving #2290

Merged
merged 1 commit into from
Nov 9, 2016
Merged

Conversation

josdejong
Copy link
Member

@mojoaxel can you have a look at this PR?

It fixes #2251.

It's a bit of a workaround, it should be able to solve this by using moment.duration (see moment.js docs: http://momentjs.com/docs/#special-considerations-for-months-and-years), but I couldn't get that working so far.

With this fix, the axis will be displayed again after daylight saving switch:

image

What's not yet very nice is that the hour 2 o clock is displayed twice (empty space would be more neat I think):

image

@itfourp
Copy link

itfourp commented Nov 9, 2016

Looks like a good solution to me. I'd personally prefer to have 2 o clock twice over an empty space but that's not for me to decide.

// correct for daylight saving
// FIXME: use this.current.add(moment.duration(this.step, 'hour'))
// see http://momentjs.com/docs/#special-considerations-for-months-and-years
if (this.current.hours() % this.step !== 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I don't understand this part. Can you explain in a few words what you are doing here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this line:

this.current.add(this.moment.duration(this.step, 'hour'));

we add for example 4 hours when this.step is 4. The resulting number of hours this.current.hours() should be a multiple of for, ie. the modulus of 4 should be zero.

What happens when stepping over daylight saving is that this.current.hours() becomes 3 instead of 4. Therefore we add the difference to the expected value 4, ie. 4-3 = add 1 hour.

Again, I would love to see this taken care of by moment.js since these are tricky things but I couldn't get that working quickly.

Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine! Thanks so much!!

@mojoaxel mojoaxel merged commit 2f77d80 into develop Nov 9, 2016
@mojoaxel mojoaxel deleted the fix/daylight_saving branch November 23, 2016 14:33
@mojoaxel mojoaxel added this to the Minor Release v4.18 milestone Dec 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants