Skip to content

Ensure all strings are updated when switching language - #504

Merged
kozer merged 6 commits into
trunkfrom
fix/language-switcher-update-translations
Sep 3, 2024
Merged

Ensure all strings are updated when switching language#504
kozer merged 6 commits into
trunkfrom
fix/language-switcher-update-translations

Conversation

@fluiddot

@fluiddot fluiddot commented Sep 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related to #476 (comment).

Proposed Changes

  • Update the default I18n locale data when switching languages. Note we need to do this for both renderer and main processes.
  • Delegate app menu setup to useI18nData hook. This way the menu items will reflect the proper translations when opening the app and switching languages.

Testing Instructions

Warning

  • Some of the menu items are not translated because they are added by the OS based on the OS language set.
  • The string Language in the Settings popup is not translated because it's not included yet in the translation files.
  • Open app settings.
  • Change the language.
  • Observe that all strings are translated to the selected language, including the app menu.
  • Re-open the app.
  • Observe that all strings are translated to the previously selected language, including the app menu.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@fluiddot
fluiddot requested a review from a team September 3, 2024 09:35
@fluiddot fluiddot self-assigned this Sep 3, 2024
@sejas sejas mentioned this pull request Sep 3, 2024
Closed
1 task
This was referenced Sep 3, 2024
Merged
Merged
kozer
kozer reviewed Sep 3, 2024
View reviewed changes

@kozer kozer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested it but I have the following problem:

  1. When I reopen the app, and switch language back to English ( It was switched to Spanish before restarting ), the menu items are not switch back. Is this expected @fluiddot ?
@kozer

kozer commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

NOTE: This is not happening switching to other languages. ( eg from French to Spanish and back to French )
cc: @fluiddot
@fluiddot

fluiddot commented Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

Tested it but I have the following problem:

  1. When I reopen the app, and switch language back to English ( It was switched to Spanish before restarting ), the menu items are not switch back. Is this expected @fluiddot ?

Good catch @kozer !

For English, we don't provide translations as we use the default strings set in the code. I thought that passing an undefined value to the setLocaleData should be enough for this case, but I noticed we need to explicitly reset it. I've pushed a fix in 4b237a0.
@fluiddot
fluiddot requested review from a team and kozer September 3, 2024 11:53
kozer
kozer approved these changes Sep 3, 2024
View reviewed changes

@kozer kozer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested it and works as expected! Thank you @fluiddot ! Nice work!
kozer
kozer reviewed Sep 3, 2024
View reviewed changes
Comment thread src/hooks/use-i18n-data.tsx
} else {
// In case we don't find translations, we reset the locale data to
// fallback to the default translations.
defaultI18n.resetLocaleData();

@kozer kozer Sep 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we remove that, as we do it via ipc?

@kozer kozer Sep 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

https://github.com/Automattic/studio/pull/504/files#diff-4897e47ea3fb6cdbb1798e8dce993fa538a1d6d56b425eabbf82d945889ec508R726

@fluiddot fluiddot Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As far as I've tested we need both. The object defaultI18n object is created globally so we need to update it for each process, the main and renderer.
@kozer

kozer commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

@fluiddot left a comment!
Other than that, it works as expected!
@kozer

kozer commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

I removed it myself in 21ddf81
@fluiddot

fluiddot commented Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

I removed it myself in 21ddf81

As I shared here, we need both. Actually, with this removal, I noticed that some of the translations are not updated when switching to English from another language.

image
@kozer

kozer commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

I removed it myself in 21ddf81

As I shared #504 (comment), we need both. Actually, with this removal, I noticed that some of the translations are not updated when switching to English from another language.

image

reverted!
@fluiddot
fluiddot requested a review from a team September 3, 2024 12:16
sejas
sejas approved these changes Sep 3, 2024
View reviewed changes

@sejas sejas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.
Thanks for adding the support for the menu items ✨

lcZ8hL.mp4
@kozer
kozer merged commit 5541e8a into trunk Sep 3, 2024
@kozer
kozer deleted the fix/language-switcher-update-translations branch September 3, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@fluiddot @kozer @sejas