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

Localize lang selectors according to the app language #6207

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

Signez
Copy link
Contributor

@Signez Signez commented Nov 10, 2024

There are two main things that are not yet translatable in Bluesky's UI:

  • Default feed names and descriptions (but this may be a can of worms to open, apart from the specific case of Following);
  • Language selectors, in the composer and in the settings screens.

As the second one is quite ironic (don't you think? 🎶) and quite visible on a daily basis, nagging at me as the main translator of Bluesky's app into French with @surfdude29, I thought it would be great to help you out and fix it myself, so you don't have to take time away from your heavy roadmap 😅


In this pull request, I use Intl.DisplayName to offer a best-effort translation of the language names, if the browser or target provides it.

Capture d’écran 2024-11-10 à 18 15 23
⬆️ Here, notice the Français language name. It would show French on main.

Of course, I took the time to fix the same problem in the Languages screen:

Capture d’écran 2024-11-10 à 18 15 14
⬆️ This screen always offered a translation for the app language, and I don't touch it to help anyone revert an unwanted change of app language settings.

Note: This is a best-effort fix: if Expo's runtime on the mobile apps do not support the Intl.DisplayNames, it will not do anything, and simply show English names as of today.

@auroursa
Copy link
Contributor

I tested Chinese, Japanese and Korean and they work perfect for me👍
image

@tkusano
Copy link
Contributor

tkusano commented Nov 11, 2024

I haven't tested it exhaustively, but I gave it a quick run in Firefox and it looks good. Because Hermes engine ,iOS and Android use, doesn't seem to support Intl.DisplayNames API, polyfilling @formatjs/intl-displaynames might be a option.

@surfdude29
Copy link
Contributor

This is a great idea @Signez 👌

Comment on lines +52 to +66
export function languageName(language: Language, appLang: string): string {
// if Intl.DisplayNames is unavailable on the target, display the English name
if (!(Intl as any).DisplayNames) {
return language.name
}

return getLocalizedLanguage(language.code2, appLang) || language.name
}

Choose a reason for hiding this comment

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

Could this be rewritten as a hook, to avoid needing to pass in langPrefs everywhere?
e.g.

export function useLanguageName(language: Language): string {
  const {appLanguage} = useLangPrefs();
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As hooks shall not be called conditionally, an hypothetical useLanguageName hook could not be used as-is inside .map calls… where most of the existing calls to codeToLanguageName are done.

So yeah, I am not convinced we would gain readability by using a hook here!

@gaearon
Copy link
Collaborator

gaearon commented Nov 23, 2024

not gonna do right now but please remind me to look next week

@Signez Signez force-pushed the i18n-language-selector branch from 693144a to 5cb5dd7 Compare November 27, 2024 14:31
@Signez
Copy link
Contributor Author

Signez commented Nov 27, 2024

Just rebased on main, pinging you this week @gaearon as you suggested ✨

src/locale/helpers.ts Outdated Show resolved Hide resolved
@Signez Signez force-pushed the i18n-language-selector branch from a14e0ed to 145a91b Compare December 26, 2024 19:53
@Signez
Copy link
Contributor Author

Signez commented Dec 26, 2024

Rebased on main, with a nasty bug (caught by @c960657) fixed.

@Signez Signez force-pushed the i18n-language-selector branch from 145a91b to 9d9232c Compare December 31, 2024 20:51
Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Nice! My main issue is that, at least in my sim tests, Intl.DisplayNames wasn't available on either android or ios. This only worked for me in my web test. You correctly handle that situation, and I want to go ahead and merge so we make progress. Hopefully that api comes to Hermes someday.

@pfrazee pfrazee merged commit 9f075b1 into bluesky-social:main Dec 31, 2024
@Signez
Copy link
Contributor Author

Signez commented Dec 31, 2024

Thanks! It was indeed what @tkusano spotted; they suggested that we could use @formatjs/intl-displaynames (and it would work), but it would be a waste to package it in every installation when there is native APIs that would allow us to use the strings already stored by the system: getDisplayLanguage(Locale locale) on a java.util.Locale on Android, and localizedString(forLanguageCode: string) on Fondation's Locale on iOS.

I thought it would be a good follow-up PR in the future, maybe contributing an Expo package (or an improvement to expo-localization) to offer this to other projects too!

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.

8 participants