Baremetrics / calendar

Date range picker for Baremetrics
MIT License
680 stars 79 forks source link

Added compatibility with Moment Timezone. #69

Closed noelherrick closed 7 years ago

noelherrick commented 7 years ago

This allows the use of this component with pages that use Moment Timezone and have a default timezone set. Without this fix, the component interprets the dates from the input fields as UTC, causing Calendar.checkDate to return erroneous results, which are then put back into the input fields as timezoneless dates. This causes some dates to be off by days.

kalepail commented 7 years ago

@noelherrick I assume you've tested this and found it to work for your own use case? We use moment TZ at Baremetrics and haven't needed this.

noelherrick commented 7 years ago

This occurs when you have a default timezone set (after you call moment.tz.setDefault("America/Chicago")). I modified your test application to include Moment Timezone and set a default timezone it to reproduce the bug and test this fix. Should I add that as well? Perhaps a button that sets a default timezone?

kalepail commented 7 years ago

Nah that's fine. I'll merge this as your implementation won't affect many folks anyway. 👍🏼

Tyler

On Wed, Nov 02, 2016 at 2:17 AM Noel Herrick

< mailto:Noel Herrick notifications@github.com

wrote:

a, pre, code, a:link, body { word-wrap: break-word !important; }

This occurs when you have a default timezone set (after you call

moment.tz.setDefault("America/Chicago")

). I modified your test application to include Moment Timezone and set a default timezone it to reproduce the bug and test this fix. Should I add that as well? Perhaps a button that sets a default timezone?

You are receiving this because you commented.

Reply to this email directly, https://github.com/Baremetrics/calendar/pull/69#issuecomment-257782021 , or https://github.com/notifications/unsubscribe-auth/AELjegoNUfX0Xe9I0Rj5NVtyF1IczAZYks5q6CrrgaJpZM4Kl3Yz .

theodorejb commented 7 years ago

@noelherrick Can you share your test code to reproduce this issue? moment.defaultZone isn't part of the public moment-timezone API, and I'd like to see if the issue can be fixed without relying on private APIs.

theodorejb commented 7 years ago

My understanding is that calling moment.tz.setDefault will automatically cause future dates to be created in the default time zone, so it's not clear how manually checking for a default and specifying it when creating a date changes anything.

I did my own test where I call moment.tz.setDefault('Asia/Srednekolymsk'); before instantiating a calendar, and it seems to work the same with and without this patch.

kalepail commented 7 years ago

@theodorejb @noelherrick That's what we do at Baremetrics.

// Set universal timezone to UTC
moment.tz.setDefault("UTC");
moment = moment.utc;