-
Notifications
You must be signed in to change notification settings - Fork 9
Included timezone shift #7
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
base: master
Are you sure you want to change the base?
Included timezone shift #7
Conversation
Includes an adjustment to take into account the potential for a timezone to be included in the DTSTART or DTEND datetime and calculates to remove the timezone, apply as GMT and allow the local browser to render appropriately for the local timezone
Christop406
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to write a test case for this? For now, you can just add it to the test cases in test/ical/test.js. I think a test case would help not break this in the future and also give some more insight into the actual issue.
| //Allow to parse as per GMT | ||
| events[event_count][k].value = getDate(undefined, value.concat('Z')); | ||
| //If a locale exists, strip preceeding / | ||
| if(params["TZID"].search(/\//) != -1){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this does exactly what you want it to do, it seems that this check will find any / in the TZID then take a substring, so, for example, wouldn't this:
America/Los_Angelesmatch for your .search(...) call? Which would then end up with merica/Los_Angeles?
I think you would just want to check if the string starts with /, which simplifies the call to if(params["TZID"].startsWith('/'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes agree re the search. I'll make the change to the code and commit the change. I'll also write a test case for it and submit. Thanks for the advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - I'm about to head on a trip for a bit over a week tomorrow so I may go quiet here, but I will try my best to come back to this promptly after my trip. I hope you have a workaround for the time being.
| params["TZID"] = params["TZID"].slice(1); | ||
| } | ||
| //Pass to toLocaleString to reduce to GMT based on timezone | ||
| events[event_count][k].value.toLocaleString(undefined, {timeZone: params["TZID"]}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this line do? toLocaleString doesn't set anything, so I think the return of this function is unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is it returns a correctly coded datetime using the correct locale.
Includes an adjustment to take into account the potential for a timezone to be included in the DTSTART or DTEND datetime and calculates to remove the timezone, apply as GMT and allow the local browser to render appropriately for the local timezone