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 react.i18next and update project keywords for multi-language support #4190

Closed
wants to merge 21 commits into from
Closed

Conversation

mj-mehdizadeh
Copy link

Pull Request: Update project keywords for multi-language support

Description:
This pull request updates the "admin-ui" keywords to support multi-language functionality. and I added the en-US, fa-IR languages, By modifying the keywords, the project becomes more discoverable and relevant to users who are searching for multi-language solutions.

Changes Made:

  • Added keywords related to multi-language support
  • Change some irrelevant keywords

Reason for the Changes:
The addition of multi-language support is an important enhancement for this project. It enables users to easily find and identify the project as a suitable solution for their multi-language requirements. With these updated keywords, the project can reach a broader audience and increase its visibility in relevant search results.

Please review and merge this pull request to incorporate the updated keywords and improve the project's discoverability and relevance.

Screenshots or Additional Context:
image
image
image
image

@mj-mehdizadeh mj-mehdizadeh requested a review from a team as a code owner May 28, 2023 20:30
@changeset-bot
Copy link

changeset-bot bot commented May 28, 2023

⚠️ No Changeset found

Latest commit: 7b16efd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@vercel
Copy link

vercel bot commented May 28, 2023

@mj-mehdizadeh is attempting to deploy a commit to the medusajs Team on Vercel.

A member of the Team first needs to authorize it.

@samuelstroschein
Copy link

I highly recommend using ids (keys) for translations. Here is an in-depth discussion and answer on why ids instead of text makes sense opral/monorepo#599.

<h2 className="inter-base-semibold">
-            {t("Opt out of sharing my usage data")}
+            {t("sharing-data")}
</h2>

In summary, using the reference text as key breaks apart as the project grows due to (but not limited to):

  • mapping reference to translated text becomes ambiguous if the reference text changes
  • multiple reference texts with different meaning map to the same translation

The management overhead of using text as ids outweighs the "but it's simpler for developers" benefit. The desire as a developer to see the reference text in source code can be achieved with vscode extensions like https://marketplace.visualstudio.com/items?itemName=inlang.vs-code-extension.

@martinejaw
Copy link

Any news here? I would like to contribute with the spanish translation

@tsvetann
Copy link
Contributor

@mj-mehdizadeh do you have any plans to complete this great PR? Do you need help?

@mj-mehdizadeh
Copy link
Author

@tsvetann I love to complete this, but it says Code owner review required, I don't know, what should I do? is there anything I can do? I thought it's needs to approved by Code owner.

@tsvetann
Copy link
Contributor

@mj-mehdizadeh I believe you've been asked to by the reviewer (@samuelstroschein ) to change the translation keys from free text to keys. Please look at his comment. So if I understand it correctly you should make this change and push again for a review. Let me know if you need further help. Thanks

@samuelstroschein
Copy link

@tsvetann I am not a medusa member/not a reviewer.

I commented to warn about the potential problems of using source text as id. If I would be a reviewer, I would request changes but I don't speak on Medusa's behalf.

@gempain
Copy link
Contributor

gempain commented Aug 7, 2023

Yes but core components of the admin UI won't be translated. I mean, someone needs to translate those at some point 🤔

@tsvetann
Copy link
Contributor

tsvetann commented Aug 7, 2023

Yes but core components of the admin UI won't be translated. I mean, someone needs to translate those at some point 🤔

Agree. But if many plugins are translated maybe this puts more pressure on the core team to take the step

@gempain gempain mentioned this pull request Aug 7, 2023
3 tasks
@olivermrbl
Copy link
Contributor

olivermrbl commented Aug 28, 2023

@mj-mehdizadeh @samuelstroschein @tsvetann @georgerock @martinejaw @gempain

Appreciate you all showing interest in the project, and huge props for this contribution, @mj-mehdizadeh 💜

My apologies for the slow response time.

We've decided to move forward with the feature and approach in this PR with only minor adjustments:

  • Changing keys from English phrases to IDs as described by @samuelstroschein in this comment
  • Adding a language selector (our designer will get back to you with this)
  • Saving the language selection in LocalStorage (in the longer term, we might move this preference to user/store settings in the DB)

This PR was closed, but @gempain continued the work in another fork.

@gempain, will you lead the implementation of these requested changes?

Also, let me know if you want to discuss anything. Happy to jump on a call.

@tsvetann
Copy link
Contributor

Thanks @olivermrbl for your attention and recognizing the community efforts. From my side the only topic is how to share the i18n instance with admin UI plugins so we can develop multilingual plugins. Thanks

@olivermrbl
Copy link
Contributor

olivermrbl commented Aug 28, 2023

how to share the i18n instance with admin UI plugins

We will add support for translation files in plugins and admin extensions, e.g. src/admin/translations/*, in the future. On build-time, these files will be merged with the core translation files. And to "share i18n instance", the idea is that the translation hooks from i18n are exported from @medusajs/admin. We haven't validated the approach, but this is how we are currently thinking about it.

Curious to hear what you think :)

@kasperkristensen will likely be able to shed some more light on this, should it be necessary.

@samuelstroschein
Copy link

@olivermrbl @kasperkristensen I might have a simpler solution for translating medusa plugins.

We faced a similar problem for inlang plugins (discussion) and settled on a Translatable return type instead of exposing an i18n instance to plugins (source code).

Translatable is a Record<LanguageTag, T> where the language tag en is always required.

Pros

  • separation of concerns. plugins can use whatever i18n approach that suits their needs
  • medusa doesn't need to maintain i18n/runtime logic
  • no complicated fallback logic because en is always defined

In summary, an API definition. Nothing more, nothing less.

Cons

  • for complicated use cases, plugins need to setup their own i18n library (or medusa offers one)

@gempain
Copy link
Contributor

gempain commented Aug 28, 2023

@olivermrbl thanks a lot for your message ! Happy to take the lead on this. I can take a call pretty much any lunch time during the week, and I think a temporary Slack channel might help if we wanted to be more efficient. I'm having a hard time getting a dev setup to work properly without changing a few things, so I'd rather make sure I haven't missed something.

I need to comment out a few things to get the DB working: the dev setup assumes someone has a Postgres running locally, but I use docker-compose to spin up the instance without customizing anything. By default, pg doesn't allow you to delete databases, so the dev migrations aren't working unless you comment out the check that tries to delete the db first.

I think the changes you ask make perfect sense and are quite common patterns, I actually happen to have the same logic in a couple of projects so it shouldn't be hard to port here.

@samuelstroschein's suggestion seems IMO a good approach. It will save the hassle of asking plugin maintainers to change translation logic whenever one day someone switches to something other than i18next.

@samuelstroschein I think the cons you listed is not really a concern: plugins should be simple and independent enough that they can handle that complexity on their own without having to alter the translation config of the parent UI. That said, a solution to this would be to use a subscriber pattern where each plugin provides a stream of translations to the admin UI, which handles the registration of those translations to the i18next instance. Using something like BehaviorSubjects from RxJS could help. Each plugin would provide a function called only once, returning an observable of translations. Every time the plugin would need to update the translations, all you'd need is to push a new value in the observable which the admin UI would be subscribed to. Something like this:

interface Translations {
  en: Record<string,string>;
  // there's probably a way to use an enum instead of string, but you get the idea
  [key: string]: Record<string,string>;
}

interface Plugin {
  initI18n(): Promise<BehaviorSubject<Translations>>;
}
const translations$ = new BehaviorSubject({ en: { hello: 'Hello' } });

export function initI18n() {
  return translations$;
}

and in the admin UI

plugins.forEach(plugin => {
  if (plugin.initI18n) {
    plugin.initI18n.subscrib(translactions => {
      // update i18n instance
    });
  }
});

Just pseudo code here of course and I haven't gone through the entire codebase to look where plugins are handled, but you get the idea. Let me know what you think !

@olivermrbl
Copy link
Contributor

Fantastic, thanks @gempain!

I can take a call pretty much any lunch time during the week

Tomorrow at 1 pm CET? I would love to hear more about your experience setting up the development environment so we can iron out all the issues and hurdles in the process.

@samuelstroschein
Copy link

samuelstroschein commented Aug 29, 2023

@gempain

The Translations interface should enforce BCP-47 language tags as keys to ensure interop/that a medusa app can access the right translations.

interface Translations {
  en: Record<string,string>;
  // there's probably a way to use an enum instead of string, but you get the idea
-  [key: string]: Record<string,string>;
+  [key: LanguageTag]: Record<string,string>;
}

We extracted our BCP-47 language tag library for types and validation which could be used, see https://github.com/inlang/inlang/tree/armageddon/source-code/core/language-tag

@gempain
Copy link
Contributor

gempain commented Aug 30, 2023

Fantastic, thanks @gempain!

I can take a call pretty much any lunch time during the week

Tomorrow at 1 pm CET? I would love to hear more about your experience setting up the development environment so we can iron out all the issues and hurdles in the process.

Works for me ! How to initiate contact ?

@gempain
Copy link
Contributor

gempain commented Aug 30, 2023

@gempain

The Translations interface should enforce BCP-47 language tags as keys to ensure interop/that a medusa app can access the right translations.

interface Translations {
  en: Record<string,string>;
  // there's probably a way to use an enum instead of string, but you get the idea
-  [key: string]: Record<string,string>;
+  [key: LanguageTag]: Record<string,string>;
}

We extracted our BCP-47 language tag library for types and validation which could be used, see https://github.com/inlang/inlang/tree/armageddon/source-code/core/language-tag

Yes but AFAIK TS won't let you use enums for index signatures. I checked your code here but this is TS syntax I've never encountered before. Interesting 😄

@olivermrbl
Copy link
Contributor

Works for me ! How to initiate contact ?

I'll reach out on X :)

@gempain
Copy link
Contributor

gempain commented Aug 30, 2023

X 😱 😅

@olivermrbl
Copy link
Contributor

@gempain, haha - we can do whatever fits you best. Discord?

@gempain
Copy link
Contributor

gempain commented Aug 30, 2023

No X works perfect, just found funny seeing someone for the first time say X instead of Twitter 😄

@samuelstroschein
Copy link

@gempain Yes but AFAIK TS won't let you use enums for index signatures.

Why do you need/want enums? A type union seems sufficient.

TS syntax I've never encountered before. Interesting 😄

The type are generated from JSON schema for runtime validation with https://github.com/sinclairzx81/typebox :)

@gempain
Copy link
Contributor

gempain commented Sep 4, 2023

@mj-mehdizadeh I'm trying to find the most efficient way to move to keys without having to manually update each line you've already configured to use t("..."). There are like 2k+ occurrences in the admin ui. Did you use a tool / script or did you literally go through every component and wrapped strings by hand ?

Edit: I was able to write a script that updates things for me using @babel/parse, works like a charm.

@olivermrbl
Copy link
Contributor

@mj-mehdizadeh, we will merge multi-language support this week – many thanks for your initial contribution. Without you and @gempain's efforts, we probably wouldn't have had an internationalized admin dashboard for a long time.

I am sure this feature will bring joy to our community and users!

@olivermrbl
Copy link
Contributor

Also, if you are up for it, we'd love to include your translations for fa-IR in a follow-up PR :)

@olivermrbl
Copy link
Contributor

The Translations interface should enforce BCP-47 language tags as keys to ensure interop/that a medusa app can access the right translations.

@samuelstroschein, this might be obvious, but how do you handle language variations e.g. en-US, en-GB using the BCP-47 specification?

@samuelstroschein
Copy link

samuelstroschein commented Sep 11, 2023

@olivermrbl "region tags" are supported by BCP-47.

BCP-47 combines "subtags" such as regions into one tag. Hence, en-US where en is a language and US is a region are valid. The best validator I have found is https://schneegans.de/lv/.

@olivermrbl
Copy link
Contributor

BCP-47 combines "subtags" such as regions into one tag. Hence, en-US where en is a language and US is a region are valid. The best validator I have found is https://schneegans.de/lv/.

Cool, thanks!

@gempain
Copy link
Contributor

gempain commented Sep 11, 2023

BCP-47 seems indeed to be the standard chosen:

Do you want to rename the default to en-US instead of en ? Also, one thing missing in this PR is a table mapping each language code to its language, for the language selection dropdown. I can keep this simple with just this:

const languages = {
  'en-US': 'English',
  'fr-BE: 'Français (BE)'
};

IMO the language should always be written in its own languages (English or English, Français for French ...).

Or is it okay to keep locales as display values for the dropdown ?

@olivermrbl
Copy link
Contributor

Do you want to rename the default to en-US instead of en ?

Sure, we can go with en-US. I like that :)

Or is it okay to keep locales as display values for the dropdown ?

Yeah, I just noticed the design had a human-readable display name. I think we should stick with just showing locales in the select.

@samuelstroschein
Copy link

Sure, we can go with en-US. I like that :)

Are you handling copy/translations differently for the US?

If not, I recommend choosing the language tag only (en). Also for other languages you are translating to. Treat subtags as supersets of a language to accustom regional differences.

@gempain
Copy link
Contributor

gempain commented Sep 11, 2023

@olivermrbl up to you, but I think @samuelstroschein suggestion makes sense.

@olivermrbl
Copy link
Contributor

Yes sure - I don’t think it matters much at this point, so please go ahead with the one you find best :)

@gempain
Copy link
Contributor

gempain commented Sep 11, 2023

Whoops I didn't notice we were in the old PR discussion. I'll move back to #4962.

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.

7 participants