Skip to content
This repository was archived by the owner on Aug 26, 2022. It is now read-only.

Fix bug 1265151 - Missing translation of some strings#3878

Merged
jwhitlock merged 13 commits intomdn:masterfrom
BychekRU:Bug-1265151
May 26, 2016
Merged

Fix bug 1265151 - Missing translation of some strings#3878
jwhitlock merged 13 commits intomdn:masterfrom
BychekRU:Bug-1265151

Conversation

@BychekRU
Copy link
Copy Markdown
Contributor

I added localization for strings 'Thanks for your feedback!' and "We
won't bug you again."

BychekRU added 5 commits May 21, 2016 17:00
I added localization for strings 'Thanks for your feedback!' and "We
won't bug you again."
Added gettext in places of call because it doesn't work in other way
$('#helpful-yes').on('click', function(e) {
e.preventDefault();
confirm('Thanks for your feedback!', 'success', 'Yes');
confirm(gettext('Thanks for your feedback!'), 'success', 'Yes');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The confirm function is currently trying to translate these strings, and will need to be modified so that double-translation is not attempted.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah. Get text should be here and not in the confirm function.

var $icon = $('<i />', { 'class': 'icon-' + sampleCodeHost, 'aria-hidden': 'true' });
// create text
var $text = 'Open in ' + this + ' ';
var $text = gettext('Open in ') + this + ' ';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should instead use ngettext, like var $text = ngettext('Open in %(site)s ', this);, so that languages where the words come in a different order will be supported.

@jwhitlock
Copy link
Copy Markdown
Contributor

@bychek-ru done with my read-through, and it looks like you are also changing the code. Mention me when you're done, and I'll get you back in the code review line.

@BychekRU
Copy link
Copy Markdown
Contributor Author

I've done and fixed all problems.

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 24, 2016

Current coverage is 85.79%

Merging #3878 into master will increase coverage by <.01%

@@             master      #3878   diff @@
==========================================
  Files           143        144     +1   
  Lines          8544       8557    +13   
  Methods           0          0          
  Messages          0          0          
  Branches       1135       1134     -1   
==========================================
+ Hits           7326       7341    +15   
+ Misses          980        979     -1   
+ Partials        238        237     -1   

Powered by Codecov. Last updated by d692d7f...73850d1

var $icon = $('<i />', { 'class': 'icon-' + sampleCodeHost, 'aria-hidden': 'true' });
// create text
var $text = 'Open in ' + this + ' ';
var $text = ngettext('Open in %(site)s ', this);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I'm new to the JS translation functions, and gave you bad advice. ngettext is for picking between a singular and a plural text (like ngettext('View the record', 'View the records', record_count)), but interpolate is used for populating a format string. I think this will work:

var $text = interpolate(gettext('Open in %(site)s '), {site: this});

@jwhitlock
Copy link
Copy Markdown
Contributor

@bychek-ru - one more change, to fix my ngettext suggestion with interpolate / gettext.

var $icon = $('<i />', { 'class': 'icon-' + sampleCodeHost, 'aria-hidden': 'true' });
// create text
var $text = ngettext('Open in %(site)s ', this);
var $text = interpolate(gettext('Open in %(site)s '), {site: this});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please replace the tabs with 4 spaces

@jwhitlock
Copy link
Copy Markdown
Contributor

Good work, @bychek-ru 👍

@jwhitlock jwhitlock merged commit 3e42c05 into mdn:master May 26, 2016
@BychekRU BychekRU deleted the Bug-1265151 branch May 26, 2016 15:43
@BychekRU BychekRU restored the Bug-1265151 branch May 26, 2016 15:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants