Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Support package.json i18n key and show a hint in case the extension is t... #7995

Merged
merged 8 commits into from
Jul 21, 2014

Conversation

marcelgerber
Copy link
Contributor

...ranslated into the users language

Implements #6835.

@njx
Copy link
Contributor

njx commented Jun 2, 2014

Cool feature! Thanks for working on this. To @dangoor

@dangoor
Copy link
Contributor

dangoor commented Jun 6, 2014

@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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@dangoor
Copy link
Contributor

dangoor commented Jun 10, 2014

Thanks for the PR, @SAplayer!

Two top-level comments:

  1. this needs unit tests. We've got a pretty extensive set of tests for this code, so it hopefully won't be too hard to add.
  2. Do you think we should display anything in the event that a package has translations but hasn't been translated into the user's language? Perhaps to encourage them to help with a translation?

@marcelgerber
Copy link
Contributor Author

@dangoor

  • LocalizationUtils (What about the name?)
  • Unit tests (Are two enough? Shall we test more scenarios? Shall we test registry extensions as well?)
  • Message for translated extensions not translated into user's language

@dangoor
Copy link
Contributor

dangoor commented Jun 27, 2014

@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);
Copy link
Contributor

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.

@marcelgerber
Copy link
Contributor Author

@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}",
Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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...

Copy link
Contributor

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.

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@dangoor dangoor added the Review label Jul 8, 2014
@dangoor
Copy link
Contributor

dangoor commented Jul 8, 2014

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)

translated_string

@marcelgerber
Copy link
Contributor Author

@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.

@larz0
Copy link
Member

larz0 commented Jul 8, 2014

@dangoor @SAplayer we can make the styling the same as keywords because keywords will be hidden once we merge #8282.

@marcelgerber
Copy link
Contributor Author

@larz0 @dangoor Changed.

@marcelgerber
Copy link
Contributor Author

@dangoor Changed once again.

@dangoor
Copy link
Contributor

dangoor commented Jul 21, 2014

@SAplayer Looks good, merging.

Would you be able to update the package format documentation?

Thanks for the work on this!

dangoor added a commit that referenced this pull request Jul 21, 2014
Support package.json i18n key and show a hint in case the extension is t...
@dangoor dangoor merged commit 0696a23 into adobe:master Jul 21, 2014
@marcelgerber marcelgerber deleted the i18n-extman branch July 21, 2014 17:40
@marcelgerber
Copy link
Contributor Author

@dangoor
Copy link
Contributor

dangoor commented Jul 21, 2014

Thanks @SAplayer!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants