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

i18n for console and FES messages #3488

Closed
wants to merge 4 commits into from
Closed

i18n for console and FES messages #3488

wants to merge 4 commits into from

Conversation

michelefenu
Copy link

This PR aims to provide a simple architecture proposal to internationalize console errors and FES messages. In reference to #3384 and #3390.

Language files are stored in src/core/i18n/ as json key/value entries. The language is selected at runtime, depending on the user browser language. If the user language is not supported (there is no dictionary for the language), then the english language file will be used.

{ "HELLO": "Hello World!!!", "HELLO_NAME": "Hello {0}!!!" }

Usage

Import the module
var i18n = require('./i18n/locales');

The simplest scenario:
console.log(i18n.__('HELLO')); // Hello World!!!

For parametric strings we need to pass the array of values as second parameter of the function:
console.log(i18n.__('HELLO_NAME', ['p5.js'])); // Hello p5.js!!!

A fallback system act as follows:

  • If the key is not found on the current language dictionary, the english dictionary is used
  • If the key is not found in the english dictionary, print the key itself

Note: this is my first contribution, all kind of feedback is welcome :)

@outofambit
Copy link
Contributor

@michelefenu thanks so much for opening this PR (and welcome to the p5js repo 💖)! I'm super excited to see some thinking and work around this topic. Will take a look this weekend and see if I have any thoughts!

@outofambit outofambit self-requested a review January 31, 2019 19:55
@michelefenu
Copy link
Author

michelefenu commented Feb 1, 2019

@outofambit thanks for the welcome! I await for your comments :)

I left things like pluralization out of this implementation to keep things simple, but if the overall structure is fine I can add them in the future.

@limzykenneth
Copy link
Member

Overall looks like a good start to me. A few minor points:

  1. Any particular reason for the exported function be named __ instead of something a bit more descriptive?
  2. You may want to consider escaping in the interpolation function. I don't think it will cause problem now but a good thing to have just in case. Can ES6's template string be utilized somehow maybe?
  3. getUserLanguage() function's method of grabbing the first part of the language code will in the most case work but in Chinese for example it won't work as the second part of the Chinese language code (CN in zh-CN, and TW in zh-TW) denote the type of script used, despite actually denoting the region. I would suggest a more fine grain and manual approach which is to use regex to pick out those that we support and know will only need their first part then if the regex didn't match, try to match those that need the whole part, then if also not match fallback to English (that reminds me that the website needs to be updated for this as well). But this is all a bit messy still.

Also consider using navigator.languages, it provides a full list of languages that the browser supports and not just the current active one.

@michelefenu
Copy link
Author

  1. Only for brevity, since it's the only exported function and the module does only one thing. But, yeah, we can definitely use a more descriptive name
  2. I think the template string might be the cleanest solution
  3. Thanks for the suggestions! If implemented properly this will not be so messy :)

@Spongman
Copy link
Contributor

Spongman commented Feb 1, 2019

IMHO better to use the named function declaration syntax function name() {} than the anonymous syntax var name = function() {} sine the former gives the functions names in the debugger.

@limzykenneth
Copy link
Member

limzykenneth commented Feb 1, 2019

Only for brevity, since it's the only exported function and the module does only one thing. But, yeah, we can definitely use a more descriptive name

Yeah then a more descriptive name would be preferred for clarity in the codebase

Thanks for the suggestions! If implemented properly this will not be so messy :)

I didn't mean your implementation is messy, just that dealing with natural languages in code is always gonna be messy. 😄

@outofambit
Copy link
Contributor

@michelefenu looks like @limzykenneth already provided some feedback 🎉 (i'm on board for everything discussed above)

@outofambit outofambit removed their request for review February 3, 2019 18:25
@outofambit outofambit added the Internationalization Relates to 'src/core/internationalization.js' label Feb 3, 2019
@michelefenu
Copy link
Author

I'm working on these improvements in a separate branch. I'm super busy these days but I plan to merge to my master branch very soon :)

@outofambit
Copy link
Contributor

hey @ermenauta i'm going to close this pull request since its been a while, but feel free to reopen it whenever you're ready to look at this again! ping me if you have any questions :) thanks!

@outofambit outofambit closed this Jul 6, 2019
@Bernice55231 Bernice55231 mentioned this pull request Mar 29, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization Relates to 'src/core/internationalization.js'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants