Added translation support #18
Conversation
|
Hey, @DJWassink. This is a bit of big change. |
|
Hey @nettofarah just touching back on this, did you have a change to look at it yet? 😃 |
|
Hey, @DJWassink. I'll be able to check it out this weekend though |
lib/translations/en.js
Outdated
| @@ -0,0 +1,32 @@ | |||
| var local = { | |||
There was a problem hiding this comment.
we need to check the indentation here.
I'm sticking to 2 spaces.
| @@ -0,0 +1,22 @@ | |||
| var get = require('lodash/get'); | |||
lib/translator.js
Outdated
| var fallbackTransMessage = get(fallbackLocals, message, message); | ||
| var transMessage = get(localization, message, fallbackTransMessage); | ||
|
|
||
| Object.keys(params).forEach(function(key) { |
There was a problem hiding this comment.
I'm trying to support ES5 with this codebase.
So we can probably swap Object.keys with some lodash helper function
There was a problem hiding this comment.
That's the funny thing, Object.keys is actually ES5 ;). Just to be sure I will swap it out with forOwn.
Maybe also better to use lodash for this, even though Object.keys is ES5 I would't be surprised if it also iterates the prototype's
lib/translator.js
Outdated
| if (typeof localization === 'undefined') { | ||
| localization = fallbackLocals; | ||
| } | ||
|
|
There was a problem hiding this comment.
let's rename trans to translated for clarity
There was a problem hiding this comment.
Could your clarify this one a bit? So far as I can see there isnt a trans variable? Or do you wanna change translator to translated?
lib/translator.js
Outdated
| var fallbackLocals = require('./translations/en'); | ||
|
|
||
| function translate(message, params, localization) { | ||
| if (typeof localization === 'undefined') { |
There was a problem hiding this comment.
we can use this here: https://lodash.com/docs/4.17.4#isUndefined
There was a problem hiding this comment.
This looks good.
Except for some style changes I requested.
Let's also keep the old english locales and revisit them in an old PR. (I would want this change to be a minor change and not introduce breaking changes just yet).
Also, can you update the README to include documentation about translations?
|
Oh, no worries then.
Let's use Object.keys
…On Fri, May 26, 2017 at 7:42 PM Dirk-Jan Wassink ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/translator.js
<#18 (comment)>
:
> @@ -0,0 +1,22 @@
+var get = require('lodash/get');
+var escapeRegExp = require('lodash/escapeRegExp')
;
+var replace = require('lodash/replace');
+var fallbackLocals = require('./translations/en');
+
+function translate(message, params, localization) {
+ if (typeof localization === 'undefined') {
+ localization = fallbackLocals;
+ }
+
+ var fallbackTransMessage = get(fallbackLocals, message, message);
+ var transMessage = get(localization, message, fallbackTransMessage);
+
+ Object.keys(params).forEach(function(key) {
That's the funny thing, Object.keys is actually ES5 ;). Just to be sure I
will swap it out with forOwn
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAQhYCGSB4L4yhGmF1rSoy55lIdk0pJ7ks5r9w8SgaJpZM4NGI6p>
.
|
|
Hey @nettofarah just checking back on what you meant with "let's rename trans to translated for clarity". I think this is the only issue remaining for a merge 🎉 🌮 |
|
@DJWassink: I meant that the name of the variable is a little weird. |
lib/translator.js
Outdated
| localization = fallbackLocals; | ||
| } | ||
|
|
||
| var fallbackTransMessage = get(fallbackLocals, message, message); |
lib/translator.js
Outdated
| } | ||
|
|
||
| var fallbackTransMessage = get(fallbackLocals, message, message); | ||
| var transMessage = get(localization, message, fallbackTransMessage); |
lib/translator.js
Outdated
|
|
||
| forOwn(params, function (value, key) { | ||
| var regex = new RegExp(escapeRegExp(':' + key), 'g'); | ||
| transMessage = replace(transMessage, regex, params[key]); |
|
@DJWassink: just left a few last comments. |
Hey guys a few days ago I added support for translation and have been running it locally here for a while and it works quite okay.
The api is pretty simple and I will give a example with the
emailvalidator.As you can see on line 56 of validations.js master...change/localization#diff-6fb54c87831f7e0eeab9cbbbc5b2b9feR56
the code looks like this:
the translator takes a message, which is either a
customMessageor a name pointing to a translation. If this name (in this case'email') is present in the translation JSON its will replace the customMessage.Without a customMessage it will take the string on: master...change/localization#diff-f06d6e49f5230c6ec1d195c064770b81R3
And the translator will try to fill the message with properties of the second argument of the translator function in the translation string, so in the email string :
'The ":paramName" must be a valid email address',:paramNamewill be replace with the paramName variable. (see the second argument in the aboven snippet{paramName: paramName})For users the default error string is already quite usable but it they want they are able to supply there own customMessage and choose themself if they use the custom properties or not.