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

Timezone Support & Several Other Fixes #28

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

liebeskind
Copy link

Adds support for timezones so will set time properly. Also included several other fixes that were previously mentioned to make setting time in calendar invite function properly.

@nwcell
Copy link
Owner

nwcell commented Sep 14, 2017

There have been a ton of changes recently and this no longer merges properly. If the merge is fixed, I'll be happy to merge this in.

@asr1
Copy link
Contributor

asr1 commented Sep 17, 2017

This pull request can be accepted instead of #43; this fixes that issue. I don't think it's possible to add this functionality without merge conflicts; the few breaking changes are necessary and also improvements wrt to time zone updates.

@nwcell
Copy link
Owner

nwcell commented Sep 20, 2017

If someone gets this merge-able, I'll merge it. It'll probably require a pull from origin... Or even a re-application of changes from the current master.

@asr1
Copy link
Contributor

asr1 commented Sep 20, 2017

I attempted--I did a pull of the most recent and applied these changes and got the same results.
I believe this file (independently) incorporates the changes that were made during the merges last week (the big one being the change from getMinutes() to getSeconds().
Obviously this PR should be tested, but I believe the way to merge these changes to is take this file over the existing ics.js file.

@nwcell
Copy link
Owner

nwcell commented Sep 20, 2017

Yeah, the last commit to that repo was in 2015 (honestly, I forgot I made this project). It either needs to be updated with more current code, or the timezone stuff needs to be re-implemented in a fresh branch.

@asr1
Copy link
Contributor

asr1 commented Sep 20, 2017

  1. Let me just say--thank you for making this, it made my life much easier in developing a tool that in turn sped up routine tasks for more than 200 people.
  2. What of this isn't compatible? It seems to be just changing the start date from new Date() to the new Date() with timezone offsets , and the changingn DATE to DATE-TIME at the end. This shouldn't affect the RRULE stuff that was added from Connorbode's branch.
    Am I missing something?

@@ -54,27 +54,30 @@ var ics = function() {
};

//TODO add time and time zone? use moment to format?
var start_date = new Date(begin);
var end_date = new Date(stop);
var offset = new Date().getTimezoneOffset();

Choose a reason for hiding this comment

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

This calculation can be incorrect due to Daylight Saving Time (DST)

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.

4 participants