-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UI: rearrange logic for adding timezone in date-format helper #26693
Conversation
Build Results: |
CI Results: |
let zone; // local timezone ex: 'PST' | ||
try { | ||
// passing undefined means default to the browser's locale | ||
zone = ' ' + date.toLocaleTimeString(undefined, { timeZoneName: 'short' }).split(' ')[2]; |
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.
removed the space here so if toLocaleTimeString
doesn't error but returns undefined
again in the future for some reason, zone
will be falsey
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.
Great! Just one suggestion to further strengthen the tests
|
||
const formatted = formatTimeZone(TEST_DATE); | ||
|
||
assert.strictEqual( |
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.
Maybe we add one more assertion, since the expected value is computed, that makes sure the zone is NOT undefined
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.
Yeah! Good call ⚡
I was unable to reproduce this issue. I downloaded Brave and set my system timezone to GMT+1. I was not able to download the older version of Brave, but at least on newer versions this is no longer an issue. I updated the logic regardless so undefined timezones won't be displayed
