Experimental language translation #3198
Conversation
G-Ambatte
added
the
P2 - minor feature or not urgent
|
The current state of this PR is that only the Account menu has been modified to use the new |
|
I could put this syntax into all texts, should i or do you think we should wait in case we want another syntax? |
I've kept it as a Draft PR for now, specifically so we could let the syntax discussion play out.
On 1. As it currently stands, if the string to be translated does not exist in the localization file, then the string itself is returned, allowing as to use the exact text as the key. That said, it doesn't HAVE to be the exact text, and where the text is particularly long, it's probably a good idea to use an Identifier instead. On 2. This seems like a good idea - as each JSX file deals with one area of the UI, we should be able to set the defaults once per file and have it "just work" from then on. Implementation, however, is usually where things fall apart. For 3. I think we need Finally, 4. Ultimately, no matter what page this is implemented on, a value needs to be manipulated and stored by the page, then retrieved and applied to the data source that the |
|
I intend to take a further stab at some of these as time allows in the next couple of days; however, given the time of year, time is a fleeting thing right now... |
It is entirely reasonable to have the source string in the code be "Error #234" and the translation be "An error occured (#234). This sometimes happens because [long reasons]. You can try [stuff]". The "identifier", per se, doesn't have to be a neologism, it can be a simple plain language string (one we'd be happy getting stuck as being permanent as a key only visible in the code .. so no rude words pls). If we do go with using strings-in-the-code as the keys-in-the-translations, we definitely should have a code-to-English translation file (and not rely on the fallback-to-key method for English UI). Otherwise we can't easily change the English UI (because it would break the keys to other langs). Shouldn't be hard to find a volunteer to write up the English translation file ;-) |
|
Quick check .. does this PR include setting the As of v3.10, THB does not assert a |
We could, not sure it is necessary, but it is best practice |
|
Last time we talked about this, we talked about what languages we could add as a start, we agreed on:
|
Engos is tongue, but if I remember correctly can also be used to mean "language"; while arilla is "beer" but might be close enough to "brews". This is why we need to finish a template file, to determine exactly what words we actually need. |
base-localization-file.zip |
|
I think in the spots where we use MomentJS to set relative times ("1 year ago"), such as in the metadata viewer in the navbar and in the Recent Items dropdown, we can use translation as well. Check out this page from MomentJS docs: http://momentjs.com/docs/#/i18n/instance-locale/ |
| {this.renderSortOption('title'.translate(), 'alpha')} | ||
| {this.renderSortOption('created date'.translate(), 'created')} | ||
| {this.renderSortOption('updated date'.translate(), 'updated')} | ||
| {this.renderSortOption('views'.translate(), 'views')} |
There was a problem hiding this comment.
Instead of translating each of these four options individually, you can instead place a .translate() on line ~125:
renderSortOption : function(sortTitle, sortValue){
return <div className={`sort-option ${(this.state.sortType == sortValue ? 'active' : '')}`}>
<button
value={`${sortValue}`}
onClick={this.state.sortType == sortValue ? this.handleSortDirChange : this.handleSortOptionChange}
>
{`${sortTitle.translate()}`} // <--------- add .translate() here
</button>
{this.state.sortType == sortValue &&
<i className={`sortDir fas ${this.state.sortDir == 'asc' ? 'fa-sort-up' : 'fa-sort-down'}`}></i>
}
</div>;
},
There was a problem hiding this comment.
i am unfamiliar with this specific code, would we keep the same key names in the yaml files?
There was a problem hiding this comment.
Yeah, the yaml's will stay the same.
Right now in the PR, the sort buttons are rendered in the renderSortOption function, and the actual text that displays on the buttons is done here:
<button ...>
{`${sortTitle}`}
</button>sortTitle is an arg of renderSortOption, and is just a string. In the PR, right now, the string that is being passed here into this arg is already translated. But it doesn't have to be. Instead, you can pass the original english string into renderSortOption. Then, we can translate sortTitle (which is just the string we are passing in).
Just for completeness, here are the two entire functions as I describe them:
renderSortOption : function(sortTitle, sortValue){
return <div className={`sort-option ${(this.state.sortType == sortValue ? 'active' : '')}`}>
<button
value={`${sortValue}`}
onClick={this.state.sortType == sortValue ? this.handleSortDirChange : this.handleSortOptionChange}
>
{`${sortTitle.translate()}`}
</button>
{this.state.sortType == sortValue &&
<i className={`sortDir fas ${this.state.sortDir == 'asc' ? 'fa-sort-up' : 'fa-sort-down'}`}></i>
}
</div>;
},
// ...
renderSortOptions : function(){
''.setTranslationDefaults(['userPage', 'filters']);
return <div className='sort-container'>
<h6>{'sortBy'.translate()}:</h6>
{this.renderSortOption('title', 'alpha')}
{this.renderSortOption('created date', 'created')}
{this.renderSortOption('updated date', 'updated')}
{this.renderSortOption('views', 'views')}
{/* {this.renderSortOption('Latest', 'latest')} */}
{this.renderFilterOption()}
</div>;
},(note, both of these have very similar function names....possibly that should be addressed but I'm not concerned about it now)
There was a problem hiding this comment.
A few comments in this review, and then a couple of separate comments because I didn't think i'd do as many as I did.
Also-- Do you have any ideas on how tooltips can be translated? It seems right now that tooltips are done entirely with LESS/CSS, going all the way to tooltip.less. Can translate() work in a LESS file?
| </div>; | ||
| return _.map(brewCollection, (brewGroup, idx)=>{ | ||
| return <div key={idx} className={`brewCollection ${brewGroup.class ?? ''}`}> | ||
| <h1 className={brewGroup.visible ? 'active' : 'inactive'} onClick={()=>{this.toggleBrewCollectionState(brewGroup.class);}}>{brewGroup.title || 'No Title'}</h1> | ||
| <h1 className={brewGroup.visible ? 'active' : 'inactive'} onClick={()=>{this.toggleBrewCollectionState(brewGroup.class);}}>{brewGroup.title || 'No Title'.translate()}</h1> |
There was a problem hiding this comment.
Right now, this does not translate "Gazook89's Published Brews" / "Gazook89's Unpublished Brews". This could be accomplished with a change on this line, and on the userPage.jsx. This will change the title of the brewCollections, so someone wiser than I (@calculuschild , @G-Ambatte for example of a non-exhaustive list) might want to confirm this doesn't pose other dangers.
But the gist of it is:
listPage line 217
<h1 className={brewGroup.visible ? 'active' : 'inactive'} onClick={()=>{this.toggleBrewCollectionState(brewGroup.class);}}>{`${brewGroup.title} ` + `${brewGroup.class} brews`.translate(['userPage']) || 'No Title'.translate(['userPage'])}</h1>userPage line 34
const brewCollection = [
{
title : `${usernameWithS}`,
class : 'published',
brews : brews.published
}
];result:
Probably there is some better way of concatenating the string together for the translate function, but right now I couldn't think of one.
There was a problem hiding this comment.
Right now, this does not translate "Gazook89's Published Brews" / "Gazook89's Unpublished Brews".
[...] Probably there is some better way of concatenating the string together for the translate function, but right now I couldn't think of one.
I suggest the translate() function include an optional parameter of key:values to be merged into the translated string. such that the brewCollection title could be translated via
"username-published-brews".translate({"name": "Gazook89"})
The english translation reference YAML file could then have for the username-published-brews key this translated value:
${name}'s published brews
or even this, to properly handle English possessive formation for names ending with "s":
${name}${name.endsWith('s') || name.endsWith('S') ? '’' : '’s'} published brews
This can handle simple substitution (e.g. inserting a name into the string), weird substitutions (e.g. irregular possessive s formation), multiple values (e.g. errorNumber, date, reference merged into a longer paragraph), and even basic math etc (e.g. ${number * factor} or string operations (e.g. "le" or "la", according to circumstance).
And, of course, handle straight up static strings (e.g."Hello!": "Bon jour!")
In the translate() function itself, it would dynamically define an anon function that returns the filled-template string (via the with (merge) statement to get the merge values into the scope).
String.prototype.translate = function (values) {
// exempli gratia..
// also ignoring the `groups` parameter functionality requirement
const translationData = {
"Names Published Brews": "${name}${name.endsWith('s') || name.endsWith('S') ? '’' : '’s'} Brassins Publiés",
"Names Unpublished Brews": "${name}${name.endsWith('s') || name.endsWith('S') ? '’' : '’s'} Brassins Non Publiés",
"Some string": "Jibber jabba",
// Add more translations as needed
};
const key = Object.keys(values)[0];
if (translationData[key]) {
// Dynamically define the function
const functionString = `with (values) return \`${translationData[key]}\`;`;
const translationFunction = new Function('name', functionString);
// Call the dynamically defined function
return translationFunction(values);
} else {
return `Translation not found for key: ${key}`;
}
};
// CODE NOT TESTEDNB. this would require a refactor of code to date, since the current translate() function already has one optional parameter which is an unlabelled array of strings for scoping the translation lookup group). I suggest instead
"source-string".translate( { values: {"key": "value", "another-key": "another value"}, groups: ["array", "of", "strings"] })
(Writing a function with an optional unlabelled parameter value is a code smell. Pass that thing inside an object with a key, please).
There was a problem hiding this comment.
OR, and just hear me out, simply change the way the userpage displays the titles, to have the username on top of the page, and then simply:
Published brews
Unpublished brews
and voilà, we just saved having to build that function at all.
There was a problem hiding this comment.
Collection titles is not the only place where we want to present a translated message that contains various values merged into it.
There was a problem hiding this comment.
I agree with both: we can easily sidestep the issue on these particular headers without losing anything, but we may still benefit from the functionality that eric proposes.
There was a problem hiding this comment.
I agree with Eric's proposal. Languages are not always going to follow the same grammatical sequence, so we need a way for each language to place passed-in values in the correct point in the text.
|
I think i have fixed most comments and reviews, except for those which require discussion or i need an answer about something, if i'm wrong, please notify me about it. |
I just figured how it works, yeah, we might have a problem there. Looking at it. |
| const languageOptions = ['pt-BR','en-US', 'es-ES', 'fr-FR']; | ||
| const langCodeCorrect =()=> { | ||
| if (languageOptions.indexOf(req.language) > -1) {return req.language;} | ||
| return 'en-US'; | ||
| }; | ||
|
|
||
| const langPreference = langCodeCorrect(); |
There was a problem hiding this comment.
I would probably just simplify this to
| const languageOptions = ['pt-BR','en-US', 'es-ES', 'fr-FR']; | |
| const langCodeCorrect =()=> { | |
| if (languageOptions.indexOf(req.language) > -1) {return req.language;} | |
| return 'en-US'; | |
| }; | |
| const langPreference = langCodeCorrect(); | |
| const langPreference = languageOptions.includes(req.language) ? req.language : 'en-US'; |
| String.prototype.translate = function(_groups = String.prototype.getTranslationDefaults()) { | ||
| const text = this.valueOf(); | ||
| const groups = _groups ? Array.from(_groups) : []; | ||
| let obj = localeData; | ||
| while (groups?.length > 0) { | ||
| if(obj[groups[0]]) { | ||
| obj = obj[groups[0]]; | ||
| groups.shift(); | ||
| continue; | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| if(obj[text] && obj[text].length > 0) return `${obj[text]}`; | ||
| return localeData[text]?.length > 0 ? `${localeData[text]}` : `${text}`; | ||
| }; |
There was a problem hiding this comment.
I would suggest we add a parameter interpolations to the translate function which is an object containing key-value pairs of any text that need to be interpolated in the text.
'UsersPublishedBrews'.translate({username : this.props.username});
Then in the YAML file, we can just add some kind of marker (I would vote the standard ${}):
UsersPublishedBrews : '${username}'s published brews'
...
UsersPublishedBrews : 'Brews publicados de ${username}'
| String.prototype.translate = function(_groups = String.prototype.getTranslationDefaults()) { | |
| const text = this.valueOf(); | |
| const groups = _groups ? Array.from(_groups) : []; | |
| let obj = localeData; | |
| while (groups?.length > 0) { | |
| if(obj[groups[0]]) { | |
| obj = obj[groups[0]]; | |
| groups.shift(); | |
| continue; | |
| } | |
| break; | |
| } | |
| if(obj[text] && obj[text].length > 0) return `${obj[text]}`; | |
| return localeData[text]?.length > 0 ? `${localeData[text]}` : `${text}`; | |
| }; | |
| String.prototype.translate = function(_groups = String.prototype.getTranslationDefaults(), interpolations = {}) { | |
| const text = this.valueOf(); | |
| const groups = _groups ? Array.from(_groups) : []; | |
| let obj = localeData; | |
| while (groups?.length > 0) { | |
| if(obj[groups[0]]) { | |
| obj = obj[groups[0]]; | |
| groups.shift(); | |
| continue; | |
| } | |
| break; | |
| } | |
| let translatedText = obj[text] && obj[text].length > 0 ? `${obj[text]}` : localeData[text]?.length > 0 ? `${localeData[text]}` : `${text}`; | |
| // Interpolate the values | |
| for (let key in interpolations) { | |
| translatedText = translatedText.replace(new RegExp(`\${${key}}`, 'g'), interpolations[key]); | |
| } | |
| return translatedText; | |
| }; |
There was a problem hiding this comment.
Then in the YAML file, we can just add some kind of marker (I would vote the standard ${}):
Why partially emulate template strings when you could implement actual template strings?
if (translationData[key]) {
// Dynamically define the function
const functionString = `with (values) return \`${translationData[key]}\`;`;
const translationFunction = new Function('name', functionString);
// Call the dynamically defined function
return translationFunction(values);
} else {
return `Translation not found for key: ${key}`;
}There was a problem hiding this comment.
Not opposed to that. But for what is essentially a find/replace action, I'm not clear what the added benefit would be given the increased complexity. (The proposed code also uses deprecated and highly discouraged syntax, so we should avoid that if we want something like this.)
There was a problem hiding this comment.
The proposal supports `${expression}`, which is more than simple substitution, and thus supports `${name.endsWith('s') ? name : name + 's'} Published Brews` (for example).
So .. odd grammar rules (like English possessive 's), or pluralisation rules, or even simply date formatting .. can all be supported via `${expression}` but not ${field}.
The general concept of dynamically compiling a function from a string is usually discouraged because you really don't want to allow user-supplied strings being compiled. In this situation though only the strings in the translation files get compiled, and they should be getting reviewed when committed.
The other issue sometimes noted is that using new Function constructors can impair code optimisation and is less performant. Unless we want to hardcode a bunch of functions for use by translate(), we're gonna have to compile on the fly.
There was a problem hiding this comment.
The points about new Function are valid and something to be careful with. But my main concern is actually the with operator, which is deprecated and should not be used.
There was a problem hiding this comment.
I might suggest lodash's _.template function which can evaluate a string with expressions like this.
There was a problem hiding this comment.
Ah .. my understanding with the with operator is that the main discouragment has to do with potentially causing unintended variable declarations and making it difficult to determine where variables are defined. Last I heard it wasn't actually deprecated, per se.
Nonetheless, I support the recommendation of _.template .. it has additional checks too (but still uses new Function as the fundamental technique, heh).
There was a problem hiding this comment.
So for reference, I believe the _.template could be used like this (last 3 lines):
String.prototype.translate = function(_groups = String.prototype.getTranslationDefaults(), interpolations = {}) {
const text = this.valueOf();
const groups = _groups ? Array.from(_groups) : [];
let obj = localeData;
while (groups?.length > 0) {
if(obj[groups[0]]) {
obj = obj[groups[0]];
groups.shift();
continue;
}
break;
}
let translatedText = obj[text] && obj[text].length > 0 ? `${obj[text]}` : localeData[text]?.length > 0 ? `${localeData[text]}` : `${text}`;
// Interpolate the values
var compiledTemplate = _.template(translatedText);
translatedText = compiledTemplate(interpolations);
return translatedText;
};
| }; | ||
|
|
||
| // Add defaults | ||
| String.prototype.getTranslationDefaults = function() { |
There was a problem hiding this comment.
I will just comment that I am not a huge fan of this groups parameter. It seems to add a layer of complexity and boilerplate code to every file:
const translateOpts = ['nav', 'saveDropdown'];
''.setTranslationDefaults(translateOpts);
Can I suggest we instead just handle the different translation groups via group.key notation?
'nav.save'.translate()
Might be a little repetitive, but should also be more legible and less chance of conflict for the pages where we are translating from multiple groups anyway.
calculuschild
added
the
🔍 R3 - Reviewed - Awaiting Fixes 🔧
|
Has this been shelved for another path? |
5e-Cleric
added
🔍 R2 - Reviewed - Needs Help 🫱
This PR resolves #2624.
This PR adds a
translatefunction to the String prototype, which when called searches a given YAML file using the string as key, returning the YAML file's value if found, and the original text if not.