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

Add support for internalization #7

Merged
merged 6 commits into from
Jul 20, 2024
Merged

Add support for internalization #7

merged 6 commits into from
Jul 20, 2024

Conversation

lebaudantoine
Copy link
Collaborator

@lebaudantoine lebaudantoine commented Jul 3, 2024

Purpose

Fix various issues related to i18n.

Proposition

  • Add an i18n dependency (i18next?)
  • Fix i18n-crowdin CI steps
  • Fix frontend-i18n-* makefile commands
  • Validate Crowdin credentials works
  • Validate backend translation works

@lebaudantoine lebaudantoine changed the title Various fixes fix various issues Jul 3, 2024
@lebaudantoine lebaudantoine changed the title fix various issues fix i18n Jul 3, 2024
@lebaudantoine lebaudantoine changed the title fix i18n add support for internalization Jul 11, 2024
@lebaudantoine
Copy link
Collaborator Author

Huge challenge to support quickly the French, English and German translations.

@lebaudantoine lebaudantoine changed the title add support for internalization Add support for internalization Jul 11, 2024
@manuhabitela manuhabitela force-pushed the fix-crowdin-ci branch 8 times, most recently from ddf3a29 to c43b0f7 Compare July 19, 2024 12:55
@manuhabitela
Copy link
Collaborator

manuhabitela commented Jul 19, 2024

So I think this is mergeable as is.

The idea is:

  • use i18next in the frontend app with a global namespace and one namespace for each "feature" dir
  • have a script that checks for all t calls in the app and extracts the missing keys in the frontend/locales/xx/*.json translation files
  • in the CI, run this script, and send the updated "source" location files to Crowdin, which are the french translations. Came to the conclusion Crowdin needed a source language and we couldn't really edit translation strings of this source language in the Crowdin UI. Might be wrong but it'll do as is for now I guess :) So we have to remember french locale strings can only be changed through the code, not Crowdin.
  • note that the crowdin "push translations" CI job is currently set to run on every PR, that might create some conflicts. I say let's handle that when that poses any problem
  • When this is merged I'll be able to better test the new GH workflow I added: it listens to a webhook configured on Crowdin, to create pull requests when new things get translated in Crowdin.

Updated CI to use "npm" instead of yarn for the frontend project based
on @manuhabitela's recommendations. Also updated the dependencies-related CI
steps that were previously missed.
Fixed a typo and ensured all instances of "crowdin"
are capitalized for consistent naming.
@lebaudantoine
Copy link
Collaborator Author

lebaudantoine commented Jul 19, 2024

Should we open an issue to keep in mind?

note that the crowdin "push translations" CI job is currently set to run on every PR, that might create some conflicts. I say let's handle that when that poses any problem

With a low priority tag

@@ -7,17 +7,24 @@
"dev": "panda codegen && vite",
"build": "panda codegen && tsc -b && vite build",
"lint": "eslint . --ext ts,tsx --report-unused-disable-directives --max-warnings 0",
"preview": "vite preview"
"preview": "vite preview",
"i18n:extract": "i18next -c i18next-parser.config.json"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am a bit lost and don't know which make commands run ... should we write a bit of documentation in /docs it would be beneficial for all projects (Impress, Regie, etc..).

I would be more than happy to take this task

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use the i18n:extract command (or make frontend-i18n-extract) if you want to generate missing translation keys in the json files (it checks all t calls in the react app). Handy while developing to make sure you didn't forget any new string to translate. But not actually necessary to use, you can also add your keys by hand while developing normally.

In the CI this is called to send translation keys to Crowdin.

Comment on lines 2 to 15
"backToHome": "",
"error": {
"heading": ""
},
"forbidden": {
"heading": ""
},
"app": "Meet",
"login": "Anmelden",
"logout": "",
"languageSelector": {
"popoverLabel": "",
"buttonLabel": ""
},
"loading": "",
"notFound": {
"heading": ""
},
"homepage": {
"heading": "",
"intro": "",
"createMeeting": "",
"login": "",
"or": "",
"copyMeetingUrl": ""
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what the best practices are for organizing translations. Any resource or tips to share?

Copy link
Collaborator

@manuhabitela manuhabitela Jul 20, 2024

Choose a reason for hiding this comment

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

I guess there are multiple ways to see it:

  • either have keys that are the actual text to translate. like Error: page not found,
  • either have keys that represent what we say. Like pageNotFound.
  • either have keys that represent in what context the string is used. Like notFound.heading
  • either have keys that are strictly tied to their use in the source code. Like layouts.NotFoundScreen.heading

First option goes a bit against how i18next works as it's more meant to be used with actual keys. So just changing a typo in an existing "key" makes us having to update the key in all languages.

I feel second option could quickly become a bit hard to manage, if lots of translation strings at the root level, maybe.

Fourth option makes translations extremely tied to our component structure. If we happen to use a different component for the not found page, we also have to change the key translation everywhere.

That's why I went with the notFound.heading style. It's a bit messier compared to fourth option I guess, but the fact our translations are not 1:1 match with components means the translations are more future proof.

And for now, all the "global" translation strings that are not really tied to a specific UI component are placed at the root of the global namespace.

We can figure this out as we go really!

src/frontend/src/locales/fr/global.json Outdated Show resolved Hide resolved
src/frontend/src/locales/fr/global.json Outdated Show resolved Hide resolved
@lebaudantoine
Copy link
Collaborator Author

Reading your PR I opened this issue #49

- dynamically load locale files for smaller footprint
- have a namespace for each feature. At first I'd figured I'd put each
namespace in its correct feature folder but it's kinda cumbersome to
manage if we want to link that to i18n management services like crowdin…
the base path is actually not a secret so we'd rather have it outside
secrets and see it easily
- upload local translation files on push
- make crowdin create a pull request when new translations are made
through the crowdin website (webhook configured on crowdin-end)
makes more sense i guess, maybe
@manuhabitela manuhabitela merged commit 789bce5 into main Jul 20, 2024
6 of 8 checks passed
@manuhabitela manuhabitela deleted the fix-crowdin-ci branch July 20, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants