-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Support package.json i18n key and show a hint in case the extension is t... #7995
Conversation
Cool feature! Thanks for working on this. To @dangoor |
@SAplayer I will get to this next week. |
|
||
// TODO: Unify with the exact same function in extensions/default/DebugCommands/main.js | ||
// New module? | ||
var getLocalizedLabel = function (locale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be pulled out somewhere else. Maybe StringUtils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we can't access the Strings from StringUtils, they are loaded later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yeah, I see that does get a bit tangled. It seems a shame to create a new "i18nUtils.js" for this one function, but I can imagine us having more functions over time and I don't like the idea of duplicating this function, small though it is.
What do you think about just having a new file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a great idea.
But I don't know about the name right now, so I'll just start with "LocalizationUtils" for now.
Thanks for the PR, @SAplayer! Two top-level comments:
|
…s translated into the users language
|
@SAplayer Sorry I didn't get to this this week. I will try to land this after getting back on July 8th. |
// List users language first | ||
var isUserLang, | ||
locales = [lang1.locale, lang2.locale]; | ||
isUserLang = locales.indexOf(lang); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit. This can be done:
var locales = [lang1.locale, lang2.locale],
isUserLang = locales.indexOf(lang);
And isUserLang
is actually the index in the index in the array. so it needs a better name like userLangIndex
or shorter if possible.
Conflicts: src/utils/StringUtils.js
@dangoor Pushed. |
@@ -472,6 +472,9 @@ define({ | |||
"EXTENSION_MORE_INFO" : "More info...", | |||
"EXTENSION_ERROR" : "Extension error", | |||
"EXTENSION_KEYWORDS" : "Keywords", | |||
"EXTENSION_TRANSLATED_USER_LANG" : "Translated into your language", | |||
"EXTENSION_TRANSLATED_GENERAL" : "Translated into some languages", | |||
"EXTENSION_TRANSLATED_LANGS" : "This extension has been translated into these languages: {0}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely need some wordsmithing here and maybe even another EXTENSION_TRANSLATED_LANGS
string for the "not translated into user's lang" case (like Contribute a translation to make the extension even more awesome
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I think that the way you have it here makes a good compromise between being concise and presenting what the user needs to know. I'd be inclined to go with this as you have it for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Translated into {1} languages
and respectively Translated into {1} languages, including yours
?
The problem about that is that we have to provide singular and plural translations...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that's nicer wording. Is it worth having to define two strings for? It's not even clear to me how many extensions will get translations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we usually use two strings everytime there's singular/plural, so I guess in this case it's the same.
And many (common) extensions already got translated, like Brackets Git, Theseus, Brackets Code Folding, HTML Skeleton, Brackets Todo, ... (OK, not too many, but those are very common)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an extension is translated into another language, that means that is is in English and in another language. So there are always at least 2 languages when it is translated. Which means that we don't really need a singular form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomMalbran Yes, it should be, but it's still possible that an extension author uses an array like ["en"]
or ["de"]
.
But I'm ok with having only a plural form, even though it will sound odd in such cases.
One minor nit: I'm not sure about the added font weight on the string about the translation. It may be better if it's just the same as keywords. @larz0 what do you think? (see screenshot below for the "translated into some languages" string) |
@dangoor I just wanted an easy way to differ keywords and the translation hint. If you know any better method, feel free to share it. |
@dangoor Changed once again. |
@SAplayer Looks good, merging. Would you be able to update the package format documentation? Thanks for the work on this! |
Support package.json i18n key and show a hint in case the extension is t...
Thanks @SAplayer! |
...ranslated into the users language
Implements #6835.