Skip to content
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

refactor(l10n): add time zone translations and update time zone data generation #2612

Merged
merged 40 commits into from
Jun 28, 2022

Conversation

ByronDWall
Copy link
Contributor

@ByronDWall ByronDWall commented May 13, 2022

Summary

This PR adds translations for long form time zone names to l10n/data/time-zones/core.json and l10n/data/time-zones/en.json. And updates the generate-l10n-data and getTimeZonesForLocale method.

Description

The generate-l10n-data script will get the most recent IANA timezone list by running moment.tz.names(), remove any time zones that are in the EXCLUDED_TIME_ZONES constant from that array, then make sure that every remaining timezone has a corresponding translation string in core.json. If a timezone is not in EXCLUDED_TIME_ZONES or core.json, the script will prompt the user to either add any "new" time zones that should be translated to core.json, or to exclude them from being added to the time zone list by adding them to the EXCLUDED_TIME_ZONES constant in src/constants.ts.

The getTimeZonesForLocale function in src/time-zone-information.ts builds a normalized object using the locale-specific translation from <locale>.json for the label value, and the offset and abbr from moment-timezone. This allows for us to generate the offset value at runtime, which will insure that the offset for all time zones that have daylight/offset time is accurate. Previously, the offset value was only set when the generate-l10n-data script was run, leading to some timezones showing inaccurate offset values when they moved from daylight to standard time or vice-versa.

A SCREEN RECORDING OF THE FINAL CLI WORKFLOW IS HERE:

Screen.Recording.2022-06-15.at.4.55.56.PM.mov

@ByronDWall ByronDWall self-assigned this May 13, 2022
@changeset-bot
Copy link

changeset-bot bot commented May 13, 2022

🦋 Changeset detected

Latest commit: 28ab4e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@commercetools-frontend/l10n Minor
@commercetools-frontend/application-components Minor
@commercetools-frontend/application-shell Minor
playground Patch
merchant-center-application-template-starter Patch
@commercetools-local/visual-testing-app Patch
@commercetools-website/components-playground Patch
@commercetools-frontend/cypress Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ByronDWall ByronDWall requested a review from a team May 13, 2022 13:35
@ByronDWall ByronDWall force-pushed the PANGOLIN-1253-timezone-handling branch from b248d2a to d64df68 Compare May 13, 2022 13:36
@github-actions github-actions bot temporarily deployed to Preview May 13, 2022 13:40 Destroyed
@github-actions
Copy link
Contributor

github-actions bot commented May 13, 2022

Deploy preview for merchant-center-application-kit ready!

✅ Preview
https://merchant-center-application-m5xw634t5-commercetools.vercel.app
https://appkit-sha-3b0bf6fc585fcc0f8bba1c73a1faa693bd1d3885.commercetools.vercel.app
https://appkit-pr-2612.commercetools.vercel.app

Built with commit 28ab4e0.
This pull request is being automatically deployed with vercel-action

Copy link
Contributor

@stephsprinkle stephsprinkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I erred on the side of caution here, feel free to disregard if too nitpicky

"America/Cancun": "Eastern Standard Time - Cancun",
"America/Jamaica": "Eastern Standard Time - Jamaica",
"America/Panama": "Eastern Standard Time - Panama",
"America/Guayaquil": "Eastern Standard Time - Guayaquil",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"America/Guayaquil": "Eastern Standard Time - Guayaquil",
"America/Guayaquil": "Ecuador Time - Guayaquil",

The regional name and clean data columns don't match on this one, is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I will have to ask Diane - we've found a few other duplicated or missed time zones, so I will add this to the list

packages/l10n/data/time-zones/core.json Outdated Show resolved Hide resolved
"America/Thunder_Bay": "Eastern Time - Thunder Bay",
"America/Toronto": "Eastern Time - Toronto",
"America/Guyana": "Guyana Time ",
"America/Asuncion": "Paraguay Time - Asuncion",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"America/Asuncion": "Paraguay Time - Asuncion",
"America/Asuncion": "Paraguay Time - Asunción",

Not sure about this one, there are others too that have been standardized out in the clean data column.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have asked Diane about accent marks as well, and will add/remove them as she requires once she gets back to me

"Europe/Prague": "Central European Time - Prague",
"Europe/Rome": "Central European Time - Rome",
"Europe/Stockholm": "Central European Time - Stockholm",
"Europe/Tirana": "Central European Time - Tirane",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The city name on this one is Tirana in the doc

"Europe/Tallinn": "Eastern European Time - Tallinn",
"Europe/Uzhgorod": "Eastern European Time - Uzhgorod",
"Europe/Vilnius": "Eastern European Time - Vilnius",
"Europe/Zaporozhye": "Eastern European Time - Zaporozhskoye",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The city name on this one is Zaporozhye in the doc

packages/l10n/data/time-zones/core.json Outdated Show resolved Hide resolved
"Asia/Baku": "Azerbaijan Time - Baku",
"Asia/Tbilisi": "Georgia Standard Time - Tbilisi",
"Indian/Mauritius": "Mauritius Time",
"Indian/Reunion": "Réunion Time",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely align on accent marks

packages/l10n/data/time-zones/core.json Outdated Show resolved Hide resolved
Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice the shape of the file looks good. Wondering how: was this generated by the script or hand? 🤔

@github-actions github-actions bot temporarily deployed to Preview May 13, 2022 17:18 Destroyed
@github-actions github-actions bot temporarily deployed to Preview May 13, 2022 17:20 Destroyed
@github-actions github-actions bot temporarily deployed to Preview May 13, 2022 17:20 Destroyed
@github-actions github-actions bot temporarily deployed to Preview May 13, 2022 17:23 Destroyed
@ByronDWall
Copy link
Contributor Author

Nice the shape of the file looks good. Wondering how: was this generated by the script or hand? 🤔

the original constant was, core.json was created by running a reduce over that constant.

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!
I left some early feedback, also to clarify a couple of things.

packages/l10n/data/time-zones/core.json Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
@ByronDWall ByronDWall force-pushed the PANGOLIN-1668-timezone-list-generation branch from c506c00 to 218e061 Compare May 16, 2022 16:18
@github-actions github-actions bot temporarily deployed to Preview May 16, 2022 16:23 Destroyed
@github-actions github-actions bot temporarily deployed to Preview May 16, 2022 16:49 Destroyed
@github-actions github-actions bot temporarily deployed to Preview May 16, 2022 16:53 Destroyed
@github-actions github-actions bot temporarily deployed to Preview May 16, 2022 18:27 Destroyed
@github-actions github-actions bot temporarily deployed to Preview May 16, 2022 18:34 Destroyed
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some quick feedback to simplify things a bit more.

packages/l10n/scripts/parse-new-time-zones.js Outdated Show resolved Hide resolved
};
/**
* TODO:
* - handle fallback to 'default'/core.json when there are no translations for a locale
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are different ways to handle this.

For example, we could remove all "empty" locale files and omit the locale in the switch-case, so that the fallback/default en locale is used.

This also means that at least the en.json should be defined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that we could lazy-load the locale first, and if the length locale.default is zero then lazy-load en, that way we wouldn't necessarily always have to define en.json

packages/l10n/src/time-zone-information.ts Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
packages/l10n/scripts/generate-l10n-data.js Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to Preview May 17, 2022 16:28 Destroyed
@github-actions github-actions bot temporarily deployed to Preview May 17, 2022 16:37 Destroyed
Comment on lines 46 to 48
title: 'Translate',
description: `Add ${timeZone} to core.json for translation`,
value: ANSWER_TYPES.ADD,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be interesting to add the option for the user to enter the text to be added in core.json for the new/unhandled time zone.

Comment on lines 96 to 110
let localeTranslations = await getImportChunk(supportedLocale);
// if there are no translations for the desired locale, fall back to en/default
if (
!localeTranslations.default ||
Object.keys(localeTranslations.default).length === 0
) {
localeTranslations = await getImportChunk('en');
}
// create time zone object with abbreviations and offsets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to handle this on a per timezone basis also and not on a chunk basis? What if we also add the new timezone name (EN version) to any <locale.>json file if the timezone is new. That would have it default to EN until we pull in something else from transifex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me if it works for you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise I don't see how we can show anything if we have say 22 translations in en.json and 20 in de.json (as two are new). I don't see how de.json being loaded then (as it's not empty) would then mean that it can never show anything for the two new TZs if I am not mistaken.

Copy link
Contributor Author

@ByronDWall ByronDWall May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know whether it'd be better to wait for a translation before making a 'new' timezone available in a locale's tz list, or to add the tz to the locale files immediately.

We can talk more about the workflow when we meet about transifex, but I think there are pros and cons to each approach. If we decide to add the translation using the CLI tool, I will refactor it so that it writes to all locale files as well as core.json.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know whether it'd be better to wait for a translation before making a 'new' timezone available in a locale's tz list, or to add the tz to the locale files immediately.

I think that's a 🐔 🥚 problem if I understand your concern right. Let's talk tomorrow.

@github-actions github-actions bot temporarily deployed to Preview May 18, 2022 10:57 Destroyed
...previousTimeZones,
[timeZoneId]: translatedTimeZones[timeZoneId]
? translatedTimeZones[timeZoneId]
: 'ADD TRANSLATION',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋🏼 When can this actually happen?

ByronDWall and others added 25 commits June 28, 2022 08:55
… from constants file to json file in l10n/data/time-zones to be able to write changes to file from cli, change timezone translations based on UX input and current moment data, begin work on updateTimezoneTranslations method of generate-l10n-data.js, run generate-l10n-data
…ta/time-zones to l10n/scripts and change it to js file, create l10n/scripts/parse-time-zones.js to handle asking cli questions about time zones, update updateTimeZoneData method to write to new translated and excluded timezones to file based on cli input in scripts/generate-l10n-data.js
…nhandled-timezones using 'prompts' to get user input, update time-zone-information.js to fallback to default/en translations when translations do not exist for a given locale and update getTimeZoneData to sort timeZones by offset
…t user for timezone translation string if adding a timezone, and a valid IANA identifier for a currently translated time zone to map to when excluding a time zone, add excluded-time-zones-fallback-map for handling excluded timezone fallbacks, add .d.ts for fallback map
…to translation map, handle adding excluded timezones to translation map, add warning when translation map ids and translated tz ids are out of sync
…ax for TRANSLATIONS_FOR_EXCLUDED_TIME_ZONES_MAP in l10n/src/time-zone-information.ts
…s, remove mock tz ids from updateTimeZoneData
@ByronDWall ByronDWall force-pushed the PANGOLIN-1668-timezone-list-generation branch from c150505 to 28ab4e0 Compare June 28, 2022 12:55
@ByronDWall ByronDWall merged commit c56498c into main Jun 28, 2022
@ByronDWall ByronDWall deleted the PANGOLIN-1668-timezone-list-generation branch June 28, 2022 13:10
@ghost ghost mentioned this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants