-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: update values for default timezone selector #17124
Conversation
b8a7fd1
to
79dde0b
Compare
Codecov Report
@@ Coverage Diff @@
## master #17124 +/- ##
==========================================
+ Coverage 76.88% 76.91% +0.02%
==========================================
Files 1031 1034 +3
Lines 55183 55398 +215
Branches 7505 7554 +49
==========================================
+ Hits 42430 42608 +178
- Misses 12501 12540 +39
+ Partials 252 250 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
A question about using Africa/Abidjan
.
const DEFAULT_TIMEZONE = 'GMT Standard Time'; | ||
const DEFAULT_TIMEZONE = { | ||
name: 'GMT Standard Time', | ||
value: 'Africa/Abidjan', |
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.
Can we use GMT
here instead? It's an official IANA time zone identifier.
If not, it might be good to say here that the Africa/Abidjan
timezone has no daylight saving, so it will be identical to GMT all year long.
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.
Gotcha, the reason that this value is used is because when we dedupe the time zone list, we choose the first value, which happens to be this one. The user never sees the value, just the label, and it has the same exact time zone match as GMT (no daylight savings time). Do you think this might pose a problem somewhere? Many of the other time zones do the same thing. PT for example is actually Vancouver.
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.
Ah, I see. I think it's fine. Maybe leave a comment saying that timezones are deduped by the first alphabetical value?
|
||
return ( | ||
<Select | ||
ariaLabel={t('Timezone')} | ||
ariaLabel={t('Timezone Selector')} |
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.
Super nit. I think we agreed that the convention should be Timezone selector
ariaLabel={t('Timezone Selector')} | |
ariaLabel={t('Timezone selector')} |
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.
thank you for pointing that out. 👍
* update values for default timezone selector * fix casing and comment * Update TimezoneSelector.test.tsx
* update values for default timezone selector * fix casing and comment * Update TimezoneSelector.test.tsx (cherry picked from commit ae4ced8)
SUMMARY
There was a bug where if GMT was selected, the value was being passed in as the readable GMT, etc which is not a valid timezone. This PR fixes that issue.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
You should be able to select GMT from the timezone dropdown in the alerts/reports modal and it should save properly and run the report with no errors.
ADDITIONAL INFORMATION