-
Notifications
You must be signed in to change notification settings - Fork 1
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 getParsedDate - add 8h to account for timezone #963
fix getParsedDate - add 8h to account for timezone #963
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 245 Passed, 36 Skipped, 49.19s Total Time |
if (format === TIMEZONE_DATE_FORMAT) { | ||
offsetDate = new Date( | ||
// Add 8 hours to account for the Singapore timezone offset | ||
new Date(dateString).getTime() + 8 * 60 * 60 * 1000, |
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.
Doesn't new Date()
by default get the date in local timezone?
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.
from what i understand, new Date()
is the reference date that provides default values for any missing components in dateString
. e.g. if the dateString
includes only the month and day, the year is taken from the reference date
let offsetDate = dateString | ||
if (format === TIMEZONE_DATE_FORMAT) { | ||
offsetDate = new Date( | ||
// Add 8 hours to account for the Singapore timezone offset |
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.
issue: I think hardcoding the 8 hours here might not be an ideal solution (although the vast majority of our users are in this timezone). Instead, we should be parsing the date in the machine's local time zone, as we can see that the date changes when you try to change your computer's timezone.
Screen.Recording.2025-01-02.at.21.49.12.mov
This doesn't affect actual live sites since the date formats have already been statically generated at build time.
Screen.Recording.2025-01-03.at.10.52.11.mov
Hence, instead of 8
, we should determine the timezone offset that the user's machine is in, then inputing that as part of the calculation here. Alternatively, we can discard the time part of this date string and only work with the date part (i.e. yyyy-MM-dd'T'HH:mm:ss.SSS'Z'
becomes just yyyy-MM-dd
) before performing the parsing, so we are guaranteed to be working without consideration for the timezone.
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.
address in 3c4c605, thanks!
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.
btw have also updated GH actions to use Asia/Singapore
during CI - c7177d5
…rong-timezone-in-renderengine-in-studio
…-renderengine-in-studio
Problem
date in collection preview is one day behind on studio
Solution
Breaking Changes
Bug Fixes:
Before & After Screenshots
BEFORE:
AFTER:
Tests