Skip to content
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

[Bug 1260987] Make "en-US" locale code not localizable #2827

Merged
merged 1 commit into from Apr 1, 2016
Merged

[Bug 1260987] Make "en-US" locale code not localizable #2827

merged 1 commit into from Apr 1, 2016

Conversation

MikkCZ
Copy link
Contributor

@MikkCZ MikkCZ commented Mar 31, 2016

No description provided.

@MikkCZ
Copy link
Contributor Author

MikkCZ commented Mar 31, 2016

I think about transforming all Contributor Satisfaction: headlines to Contributor Satisfaction: %(area)s, where the area of satisfaction will be a standalone localizable string, probably already localized elsewhere.

@MikkCZ
Copy link
Contributor Author

MikkCZ commented Mar 31, 2016

Should do the above mentioned as part of this PR? If not here or not at all, r?

@@ -16,7 +16,7 @@
<div class="inline-controls">
<div>
<select id="toggle-cohort-type">
<option value="contributor:kb:en-US">{{ _('en-US') }}</option>
<option value="contributor:kb:en-US">{{ '%(locale)s'|format(locale='en-US') }}</option>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be just "en-US". As a string. I think the formatting is not needed and also a comment should be there about it not available for localization.

@MikkCZ
Copy link
Contributor Author

MikkCZ commented Mar 31, 2016

PR updated.

@@ -16,7 +16,8 @@
<div class="inline-controls">
<div>
<select id="toggle-cohort-type">
<option value="contributor:kb:en-US">{{ _('en-US') }}</option>
<!-- The locale code is not localizable - the data are for English only. -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend jinja template comment should be something like
{# Some Comment #}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to change this to a jinja comment

@safwanrahman
Copy link
Contributor

I have reviewed the patch and seems like it needs some minor correction.
without the correction, r+wc

@rehandalal As you have worked closely with it, can you please review?

@safwanrahman
Copy link
Contributor

r+ from me.
@rehandalal can merge?

@rehandalal
Copy link
Contributor

thanks! r+

@rehandalal rehandalal merged commit bb1cd35 into mozilla:master Apr 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants