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

"Add to Calendar" creates entries at the wrong time #1068

Closed
lrasmus opened this issue Feb 10, 2023 · 12 comments
Closed

"Add to Calendar" creates entries at the wrong time #1068

lrasmus opened this issue Feb 10, 2023 · 12 comments

Comments

@lrasmus
Copy link
Contributor

lrasmus commented Feb 10, 2023

This surfaced as part of PR#1555, but appears to be more widespread. The "Add to Calendar" button creates entries, but they appear on my calendar at the wrong time. Here's an example for a meeting on 2023-02-10. It shows up correctly in the site as occurring at 1pm Central time, but the calendar entry is created for 12pm Central time.

image

@github-actions
Copy link
Contributor

Welcome to US-RSE! We are excited that this is your first issue!

@danielskatz
Copy link
Contributor

I think you meant to say "but the calendar entry is created for noon Central time."

@lrasmus
Copy link
Contributor Author

lrasmus commented Feb 10, 2023

I did, corrected, sorry and thank you @danielskatz!

@lrasmus
Copy link
Contributor Author

lrasmus commented Feb 12, 2023

From the non-minimized code posted in PR#1058, if you call the addToCalendar function and specify in the data a timezone, it ultimately uses this function n and adjusts the original date/time that you provided to offset it. For example, I am in Central timezone, if you pass "America/New_York", it will subtract an hour from the date that it gets. I think this is the problem - it explains why when I download a calendar it is an hour off.

        n = function(e, t) {
            if (e) {
                if (t) {
                    var a = new Date(e.toLocaleString("en-US", {
                            timeZone: t
                        })),
                        A = e.getTime() - a.getTime();
                    return new Date(e.getTime() + A)
                }
                return e
            }
        },

I think the solution is to remove line 28 from _includes/buttons/add-to-calendar-button.html https://github.com/USRSE/usrse.github.io/blob/main/_includes/buttons/add-to-calendar-button.html#L28

    // Event timezone. Will convert the given time to that zone
    timezone: "America/New_York", 

I can see why this parameter was included, the documentation for this plugin we're using (I think this is the right one): https://www.addevent.com/documentation/add-to-calendar-button recommends providing it. However, we provide our dates/times in UTC, so I don't think any adjustment is needed.

Local testing after removing that timezone parameter showed events being created with the correct time with this change applied. Am happy to proceed with PR, but wanted to first ask if anyone with more historical knowledge of the site can say if this will break something that I'm not thinking of?

@exoticDFT
Copy link
Member

From the non-minimized code posted in PR#1058, if you call the addToCalendar function and specify in the data a timezone, it ultimately uses this function n and adjusts the original date/time that you provided to offset it. For example, I am in Central timezone, if you pass "America/New_York", it will subtract an hour from the date that it gets. I think this is the problem - it explains why when I download a calendar it is an hour off.

        n = function(e, t) {
            if (e) {
                if (t) {
                    var a = new Date(e.toLocaleString("en-US", {
                            timeZone: t
                        })),
                        A = e.getTime() - a.getTime();
                    return new Date(e.getTime() + A)
                }
                return e
            }
        },

I think the solution is to remove line 28 from _includes/buttons/add-to-calendar-button.html https://github.com/USRSE/usrse.github.io/blob/main/_includes/buttons/add-to-calendar-button.html#L28

    // Event timezone. Will convert the given time to that zone
    timezone: "America/New_York", 

I can see why this parameter was included, the documentation for this plugin we're using (I think this is the right one): https://www.addevent.com/documentation/add-to-calendar-button recommends providing it. However, we provide our dates/times in UTC, so I don't think any adjustment is needed.

Local testing after removing that timezone parameter showed events being created with the correct time with this change applied. Am happy to proceed with PR, but wanted to first ask if anyone with more historical knowledge of the site can say if this will break something that I'm not thinking of?

@lrasmus Great catch! I tested this change locally and everything seems to work on my local setup. This should not affect anything other than the button this file creates. So, should be safe to modify.

My only potential concern is if someone has their calendar app setup to use a different timezone by default than what the browser thinks, we may still run into some conflicts. It may be worth trying to set this timezone variable with the same timezone that is used for the timezone string that appears in events. In other words, maybe using something similar to code in _includes/events/event-time.html, specifically line 16 - var tz = Intl.DateTimeFormat().resolvedOptions().timeZone; This file is what sets the time stamp we put before the "Add to Calendar" button.

@exoticDFT
Copy link
Member

Actually, if you simply replace line 28 with timezone: Intl.DateTimeFormat().resolvedOptions().timeZone, instead of timezone: "America/New_York", it seems to get the correct timezone and states that in at least Google Calendar.

By removing the timezone line entirely, I was not seeing a timezone stated in Google Calendar, which I think would result in it defaulting to whatever the individual's Google Calendar is set too. And 99% of the time, I'm sure that would be the timezone the browser would also think. But seems worth enforcing it to match what the event's page states above the button.

@lrasmus
Copy link
Contributor Author

lrasmus commented Feb 15, 2023

Thank you @exoticDFT, it's a great suggestion and it seems to work in my local testing too. Do you want to make the change/PR with suggested fix? I feel like you're the one who really "solved" this, and I would hate to take credit unfairly.

@exoticDFT
Copy link
Member

@lrasmus feel free to make the PR with that change, if you would like. I would have never looked into it without you recognizing the problem in the first place. I'd say this is definitely yours to claim 🙂

@danielskatz
Copy link
Contributor

Let's go ahead and do this, then we can also merge #1055 and delete #1058 (unless we resolve this through that)

lrasmus added a commit to lrasmus/usrse.github.io that referenced this issue Feb 16, 2023
…y creation

Actual fix suggested by @exoticDFT.  Previously "America/New_York" was passed as the time zone to create calendar entries from the "Add to Calendar" button, however this was generating the calendar entry then with the wrong local time (e.g., it was set an hour off in a non-Eastern time zone).  Instead, pass the user's timezone.
lrasmus added a commit to lrasmus/usrse.github.io that referenced this issue Feb 16, 2023
The "Add to Calendar" button (per issue USRSE#1068) was creating entries anchored in "America/New_York" which meant it was shifting the time to the other time zone for non-Eastern TZ folks.  This uses the current time zone for the user.

Credit to @exoticDFT for the actual code fix to use
@lrasmus
Copy link
Contributor Author

lrasmus commented Feb 16, 2023

@danielskatz - this incorporates the fix from @exoticDFT for the calendar issue. I agree to delete #1058 and if we're good with everything here we can get #1055 merged.

@danielskatz
Copy link
Contributor

danielskatz commented Feb 16, 2023

@lrasmus - It feels a bit wrong to me to have these fairly different changes in the same PR in terms of someone wanting to look at the calendar code in the future. I was expecting a new PR for that, that we would merge, then we would create the new events by merging #1055.

What do you think? I could be convinced this way is ok too...

@lrasmus
Copy link
Contributor Author

lrasmus commented Feb 16, 2023

I'm good with whatever is preferred - I can definitely see the logic behind 2 PR and separation

lrasmus added a commit to lrasmus/usrse.github.io that referenced this issue Feb 16, 2023
lrasmus added a commit to lrasmus/usrse.github.io that referenced this issue Mar 6, 2023
The "Add to Calendar" button (per issue USRSE#1068) was creating entries anchored in "America/New_York" which meant it was shifting the time to the other time zone for non-Eastern time zone folks.  This uses the current time zone for the user.

Credit to @exoticDFT for the actual code fix to use
danielskatz pushed a commit that referenced this issue Mar 6, 2023
…1085)

The "Add to Calendar" button (per issue #1068) was creating entries anchored in "America/New_York" which meant it was shifting the time to the other time zone for non-Eastern time zone folks.  This uses the current time zone for the user.

Credit to @exoticDFT for the actual code fix to use
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

No branches or pull requests

3 participants