-
-
Notifications
You must be signed in to change notification settings - Fork 79
Fixes various inconsistencies and outdated mappings in timezone data #585
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: main
Are you sure you want to change the base?
Fixes various inconsistencies and outdated mappings in timezone data #585
Conversation
Timezone names for various cities were outdated, and in some places, wrong. These are fixed now with correct mapping.
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.
Thanks for cleaning these up! Just added a few suggestions.
packages/timezone-data/src/index.ts
Outdated
{ | ||
name: 'America/Tijuana', | ||
label: '(GMT -8:00) Chihuahua, La Paz, Mazatlan' | ||
}, | ||
{ | ||
name: 'America/Los_Angeles', | ||
label: '(GMT -8:00) Pacific Time (US & Canada); Tijuana' | ||
}, |
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.
suggestion (blocking): Instead of removing the America/Tijuana
item, which could cause backwards compatibility issues (and removes a valid time zone), can we just remove Tijuana from the America/Los_Angeles
item and fix the America/Tijuana
label?
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.
Makes sense, I've updated with this change.
name: 'Asia/Krasnoyarsk', | ||
label: '(GMT +7:00) Krasnoyarsk' | ||
label: '(GMT +7:00) Krasnoyarsk,Novosibirsk' |
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.
suggestion (blocking): Instead of adding Novosibirsk here to Asia/Krasnoyarsk
, can we add Asia/Novosibirsk
as its own item?
I did some brief reading and the history is pretty confusing, but I think they each have their own time zone identifier even if they currently observe the same times year-round.
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 initially considered adding Asia/Novosibirsk
as its own item, but felt the current grouping better aligns with the original intent behind this curated timezone list (see historical context section).
In short, here's my thinking:
- The list was merged in May 2016 when
Novosibirsk
had a different timezone. It moved to its current timezone shortly after (in July 2016), but the change wasn’t reflected here. - The list is intentionally kept small to avoid overwhelming users. Extra items add up over time.
- We already group cities that share a timezone. Examples:
- Rome is grouped under
Europe/Amsterdam
, even though it has its own canonical identifierEurope/Rome
. - Madrid is grouped under
Europe/Paris
, even though it has its own canonical identifierEurope/Madrid
.
- Rome is grouped under
/settings
has search for the timezone dropdown, so typing “Novosibirsk” will still find it even if it's grouped and not a separate item.- Grouping by country/timezone feels logical, and a separate entry would mainly help with auto-timezone detection, but even that could be implemented without adding new items.
That’s my reasoning, but I’m happy to revisit if you feel strongly about keeping Novosibirsk separate.
{ | ||
name: 'America/Los_Angeles', | ||
label: '(GMT -8:00) Pacific Time (US & Canada); Tijuana' | ||
}, | ||
{ | ||
name: 'America/Mazatlan', | ||
label: '(GMT -7:00) La Paz, Mazatlan' |
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.
nit: it might make sense to put the city that matches the time zone first (especially since there's two different "La Paz" in this file), unless there's a reason for doing it this way i'm not aware of.
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.
Thanks for the suggestion. The timezone isn’t exposed to the user in the frontend, only the label is. The cities in the label follow alphabetical order, and putting Mazatlan in front would break this order. Was there a specific reason you think putting the city matching the timezone first would work better here?
@@ -185,39 +193,40 @@ const timezoneData: {name: string; label: string}[] = [ | |||
}, | |||
{ | |||
name: 'Asia/Kolkata', | |||
label: '(GMT +5:30) Chennai, Calcutta, Mumbai, New Delhi' | |||
label: '(GMT +5:30) Chennai, Kolkata, Mumbai, New Delhi' |
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.
nit: i know this list was already existing, but same suggestion here about putting the city that matches the time zone first in the list (again, unless there's a reason for the order)
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.
Similar to my earlier comment, the cities here are in alphabetical order, so putting Kolkata first would break that.
…te entry; this is needed for backward compatibility
Implements the suggested fixes in TryGhost/Ghost#24536.
Ghost admin tests are updated in TryGhost/Ghost#24562.
Summary of changes:
America/Tijuana
entry, asTijuana
is already covered under Pacific Time.Chihuahua
usingAmerica/Chihuahua
, as the current timezone mapping is incorrect.La Paz
andMazatlan
usingAmerica/Mazatlan
, as the current timezone mapping is incorrect.Georgetown
under GMT-4, along with Caracas and La Paz. All three cities observe GMT-4 throughout the year, and don't observe DST.Almaty
andAstana
under a new timezone identifier, as the current timezone mapping is incorrect.Calcutta
(former city name) to the current nameKolkata
.Rangoon
(former city name) to the current nameYangon
.Katmandu
(former city name) to the current nameKathmandu
.Ulaan Bataar
to the correct nameUlaanbataar
.Asia/Colombo
, as the current timezone mapping is incorrect.