Skip to content
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

Fix incorrect date header when in a timezone with +/- 30 mins #56

Merged
merged 1 commit into from Jul 29, 2013

Conversation

pedrosland
Copy link
Contributor

Date headers should be like
Tue, 15 Jan 2013 14:45:11 -0430
but are like
Tue, 15 Jan 2013 14:45:16 -0450

For info on a time zone with a 30 min difference see http://www.timeanddate.com/library/abbreviations/timezones/sa/vet.html

The original bug for including the date header had it correct with dateFormat but neither the Z or ZZ in moment are quite the same. My proposed fix isn't very elegant but it seems to work.

@eleith
Copy link
Owner

eleith commented Jul 23, 2013

why do you think it isn't very elegant?

@pedrosland
Copy link
Contributor Author

It just feels a little awkward to have to ask for another date format and then use replace on it. It would be prettier just to do .moment('ddd, DD MMM YYYY HH:mm:ss X') or something but I don't think we want to add another placeholder to moment.js.

Note: X = unix timestamp in moment.js.

@eleith
Copy link
Owner

eleith commented Jul 23, 2013

why does moment.format("ZZ") not work?

@eleith
Copy link
Owner

eleith commented Jul 23, 2013

also, what about using moment().zone() ? http://momentjs.com/docs/#/manipulating/timezone-offset/

@pedrosland
Copy link
Contributor Author

RFC2822 says that time zones should be hhmm (page 14). The information here under "Email time zone indicator" supports my interpretation.

ZZ doesn't work as it gets the offset in hhff where ff = fraction of hour (50 instead of 30 minutes).

We could calculate it by getting the current zone (.zone()) and then extracting hours and minutes but personally, I prefer the .replace() method.

I created a fiddle to show this in action http://jsfiddle.net/pedro_sland/dzkqV/1/.

@eleith eleith merged commit 9599399 into eleith:master Jul 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants