-
Notifications
You must be signed in to change notification settings - Fork 400
Add i18n.addonType helper #2979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
This looks good, but while you're at it could you use this helper in the places we're already translating the entire string to include the addonType
? I think it's largely just in the "leave a review" section of the add-on detail page.
src/core/i18n/utils.js
Outdated
oneLineTranslationString(pluralKey), val); | ||
}; | ||
|
||
// Because the `addonType` we use internally is not translated, we needs this |
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.
Nitpick editorialising, but this could be:
// The `addonType` we use internally is not translated, so we use this
// helper to return translated add-on type names.
tests/unit/core/i18n/test_utils.js
Outdated
expect(i18n.addonType(ADDON_TYPE_LANG)).toEqual('language pack'); | ||
expect(i18n.addonType(ADDON_TYPE_OPENSEARCH)).toEqual('search plugin'); | ||
expect(i18n.addonType(ADDON_TYPE_THEME)).toEqual('theme'); | ||
// this one seems deprecated |
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.
It is but it will still appear for older (ESR) clients I think? I would include it as "theme" as well.
return i18n.gettext('language pack'); | ||
case ADDON_TYPE_OPENSEARCH: | ||
return i18n.gettext('search plugin'); | ||
case ADDON_TYPE_THEME: |
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.
I think you can just add case ADDON_TYPE_COMPLETE_THEME:
here; as long as we "support" it we should return a value.
This is my first time seeing mozilla/addons#10681 . I don't think it's safe to replace |
As mentioned on IRC we actually shouldn't do this, as advised by localisers. There are too many edge-cases we can't anticipate. Sorry about that @willdurand 😢 |
Fix mozilla/addons#10681