feat: allow tokens in localize, localize progress bar time #4060

gkatsev · 2017-02-10T23:43:21Z

OwenEdwards · 2017-02-11T19:17:40Z

src/js/component.js

-      return primaryLang[string];
+    if (tokens) {
+      localizedString = localizedString.replace(/\{(\d+)\}/g, function(match, index) {
+        return tokens[index - 1];


Do we need to check the validity of index here (is a number, is in-range for tokens[], etc.)?

Yeah, we probably should. Should we just return the match if the index isn't available in tokens[]?
So, if you do this.localize('{1} {2} {3}', ['a', 'b']) you would get a b {3}.

Seems like a reasonable default behavior. Of course, we're also going to need to clearly document what {1}, {2} etc. are for each string.

Just looking at this, I realize that the visual display next to the progress bar is actually the remaining time, not the duration of the video. Maybe {3} should be remaining time, so that the text can be customized that way, too?

Of course, we're also going to need to clearly document what {1}, {2} etc. are for each string

I've been planning to write a glossary explaining every string in en.json. The translations have grown a lot recently and there's a lot in there that's not all that clear out of context.

Another thing I can think of to help with this is having the translation string and then default be two different things. So, for this case I can do something like this.localize('progress bar timing', [currentTime, duration], '{1} {2}') and the en.json would have progress bar timing: '{1} {2}'. That way we have a default, that's described, but people who only look at the json would have a better idea about what's happening.
Not quite sure if there's the best approach but a thought.

To elaborate and update (after some discussion with @OwenEdwards).
The key should be progress bar timing: currentTime={1}, duration={2} and the value would be {1} of {2}. That way, as you're reading the JSON files, you have some idea of what the items are.
Then, we would want to add a 3rd argument to localize, a defaults argument, which would be {1} of {2} which is what should be used in the case a language file isn't included (and so we don't have to require en.json to be included).

OwenEdwards · 2017-02-11T22:49:41Z

Tested with IE11 + JAWS, and macOS 10.12.3 Safari + VoiceOver; other than the notes I've already made, LGTM

misteroneill · 2017-02-13T23:17:22Z

src/js/component.js

@@ -220,28 +220,35 @@ class Component {
   * @return {string}
   *         The localized string or if no localization exists the english string.
   */
-  localize(string) {
+  localize(string, tokens, defaultValue = string) {


Don't forget to update the JSDoc block for this function.

oops, will do.

do we actually need "defaultValue" as an argument here? Can't we just make a copy of string in the function

yes, we need defaultValue as an argument. The = string just makes the default value of defaultValue to be string. But if a defaultValue is passed in to this function, it is used instead of the value of `string.

I tried explaining it in the jsdoc, can you re-read it and let me know?

Ah, yes, makes sense to me. 👍

Why wouldn't we just make "{1} of {2}" the string we are localizing here? Seems weird to make the translation key and the value have different wording.

So that the key in the lang file describes what it should be. If you see {1} of {2} how would you know what is {1} and {2} and also what should you be translating?

hmmm ok LGTM

Updated

brandonocasey

1 comment LGTM otherwise

brandonocasey · 2017-02-16T19:52:10Z

src/js/component.js

@@ -220,28 +220,35 @@ class Component {
   * @return {string}
   *         The localized string or if no localization exists the english string.
   */
-  localize(string) {
+  localize(string, tokens, defaultValue = string) {


do we actually need "defaultValue" as an argument here? Can't we just make a copy of string in the function

gkatsev added the a11y This item might affect the accessibility of the player label Feb 10, 2017

gkatsev requested a review from OwenEdwards February 10, 2017 23:44

OwenEdwards reviewed Feb 11, 2017

View reviewed changes

misteroneill previously requested changes Feb 13, 2017

View reviewed changes

gkatsev force-pushed the localize-tokens branch from e813c00 to 6022b0b Compare February 16, 2017 19:00

brandonocasey reviewed Feb 16, 2017

View reviewed changes

brandonocasey approved these changes Feb 21, 2017

View reviewed changes

misteroneill approved these changes Feb 21, 2017

View reviewed changes

misteroneill added the confirmed label Feb 21, 2017

gkatsev added 7 commits February 21, 2017 16:01

add simple tokenization support to localization

dbd89aa

add a languages watcher

ce6a4ff

localize value text for progress bar with token replacement

9925b3f

add a defaultValue to localize so we can use a different key

0dbae57

use match if tokens[index - 1] doesn't exist

2ad2c1f

add jsdoc comment

d7da19d

update translations-needed

753cfa6

gkatsev force-pushed the localize-tokens branch from 6022b0b to 753cfa6 Compare February 21, 2017 21:03

gkatsev merged commit db01120 into master Feb 21, 2017

gkatsev deleted the localize-tokens branch February 21, 2017 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow tokens in localize, localize progress bar time #4060

feat: allow tokens in localize, localize progress bar time #4060

gkatsev commented Feb 10, 2017

OwenEdwards Feb 11, 2017

gkatsev Feb 11, 2017

OwenEdwards Feb 11, 2017

OwenEdwards Feb 11, 2017

mister-ben Feb 13, 2017

gkatsev Feb 13, 2017

gkatsev Feb 13, 2017

OwenEdwards commented Feb 11, 2017

misteroneill Feb 13, 2017

gkatsev Feb 13, 2017

gkatsev Feb 14, 2017

brandonocasey Feb 16, 2017

gkatsev Feb 16, 2017

gkatsev Feb 16, 2017

misteroneill Feb 16, 2017

brandonocasey Feb 16, 2017

gkatsev Feb 16, 2017

brandonocasey Feb 21, 2017

brandonocasey left a comment

brandonocasey Feb 16, 2017

feat: allow tokens in localize, localize progress bar time #4060

feat: allow tokens in localize, localize progress bar time #4060

Conversation

gkatsev commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenEdwards commented Feb 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment