i18n: Add full i18n support #84

mooncos · 2017-12-13T22:05:55Z

This adds i18n support for the app, being
rendered in client-side so that translations
can be loaded dynamically. Implemented language
fallbacks for non-existing translations, plurals
and dynamic placeholders for strings.

Fixes #38

blazeu

Rather than storing the vendor js files in our repo, load it from somewhere, like cdnjs, jsdelivr or unpkg.

This will avoid the need to ignore those files in our linter, and it's the best that we can do before implementing a build system that takes care of npm/js package in the browser.

blazeu · 2017-12-14T05:24:08Z

.coafile

@@ -1,6 +1,6 @@
 [all]
 files = **.js, **.json, **.css, **.yml, **.html, .eslintrc
-ignore = node_modules/**, out/**, package-lock.json
+ignore = node_modules/**, out/**, package-lock.json, static/js/lib/**


Don't do this please, we want those files to be linted

jayvdb · 2017-12-14T05:39:54Z

static/js/i18n/hi.json

+  "last-updated": "आखरी अपडेट",
+  "order-info": "प्रत्येक संगठन (और उनके GitHub खाते, यदि लागू हो) के लिए प्रमुख प्रतिभागी क्रमशः सूचीबद्ध होते हैं",
+  "tasks-completed": "पूरा कार्य",
+    "footer-text": "Google Code-in और इसके लोगो Google Inc. के ट्रेडमार्क हैं",


indent problem.

jayvdb · 2017-12-14T05:43:11Z

static/js/i18n/id.json

+  "tasks-completed": "Tugas selesai",
+  "footer-text": "Google Code-in dan logonya adalah merek dagang terdaftar dari Google Inc.",
+  "orgs-no-listed": "Organisasi tanpa pemimpin yang terdaftar",
+  "passed": "$1% passed, approximately $2 before task claiming ends",


if it isnt translated, it should be omitted, so that we can build checks for percentage translated, and tools to create translations.

does jquery.i18n not support fallback to English for missing messages?

If it doesnt, please dont include languages which you dont know your translation is 100% accurate and complete. We can use http://hosted.weblate.org/ to manage the collection of translations and ensure they are complete before the file is available for the i18n layer.

jayvdb · 2017-12-14T05:48:52Z

static/js/i18n/hi.json

@@ -0,0 +1,21 @@
+{
+  "page-title": "Google Code-in 2017 वर्तमान नेता",
+  "last-updated": "आखरी अपडेट",


where did you get this translation from?

Is that translation MIT licensed per #6?
If not, please remove, and we'll acquire the translation again using a method that complies with the intended license of this repo.

IMO we should remove all translation for now and only keep en or 2.

jayvdb · 2017-12-16T03:34:22Z

.coafile

@@ -9,7 +9,7 @@ bears = SpaceConsistencyBear
 default_actions = *: ApplyPatchAction

 [all.linelength]
-ignore += **.html
+ignore += **.html, static/js/i18n/**.json


ugh? Why is this?

They should all be unicode, and should all be nicely structured JSON.

JSON doesn't have linebreaks and the translations for some strings are too long for the linelength limit.

jayvdb · 2017-12-16T03:36:37Z

templates/main.html

                />
              </a>
            {{/github}}
            {{#mailing_list}}
              <a href="{{mailing_list}}">
-                <img src="images/mail.png" class="chat" alt="Join their mailing list" />
+                <img src="images/mail.png" class="chat" />


why removing all the alt?
jquery.i18n doesnt support them?
English only alt is better than no alt..?

may also be related to the #78 merge

jayvdb · 2017-12-16T03:37:52Z

templates/main.html

  <head>
    <meta charset="utf-8">
    <title>GCI Leaders</title>
-    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no">


This looks like it is doing something unrelated to the issue you are solving..?
please correct me if I am wrong about that.

Looks like it was related to https://github.com/coala/gci-leaders/pull/78/files#diff-c8935f416a66e3b982a42dccd8206926L6

jayvdb · 2017-12-16T03:39:56Z

templates/main.html

@@ -1,22 +1,33 @@
 <!DOCTYPE html>
-<html lang="en">
+<html>


lang="mul".

If not, it should still be lang=en, because it is only with JavaScript enabled that it is multilingual, and the JavaScript should update the lang attribute as required.

jayvdb · 2017-12-16T03:48:29Z

unack 911e838

mooncos · 2017-12-16T08:08:27Z

Sorry those were conflicts with rebasing from master as main.html was changed. Will fix now.

blazeu · 2017-12-17T10:01:36Z

Can you add a languange switcher? Maybe using <select>

blazeu · 2017-12-17T13:50:18Z

static/js/global.js

@@ -0,0 +1,24 @@
+var browser_locale = window.navigator.language.split('-')[0]


camelCase please

oops too accustomed to python hehe

blazeu · 2017-12-17T14:02:08Z

static/js/global.js

+  updateTranslation(browser_locale)
+  $('#lang-select').val(browser_locale)
+  $('#lang-select').on('change', function() {
+    updateTranslation(this.value)


It would be nice if the user's preference is saved (preferably in localStorage), so that the user doesn't need to change it on every visit.

Also why is this not in the other file? (language_switcher)

blazeu · 2017-12-17T14:30:55Z

templates/main.html

-    <script src="js/app.js"></script>
+    <script>
+      $(window).on('load', function() {
+        $.getScript('js/app.js')


I saw you were using defer before, why change?

defer was not working properly. I implemented this workaround to make sure that the translation file finished importing before applying the translations

translation file finished importing

Then it's not $(window).on('load'), there must be another event from jquery.i18n

It needs to because the text to translate (the HTML) needs to load as well so $(window).on('load') is the correct way.

blazeu · 2017-12-17T14:36:24Z

static/css/main.css

+
+.chooser {
+  margin-top: 0.5em;
+  display: flex;


I don't think you need flex

blazeu · 2017-12-17T14:41:01Z

static/js/global.js

+      console.log('i18n:' + localex + ' locale loaded')
+      $('body').i18n()
+      $('html').attr('lang', localex)
+      $.getScript('js/app.js')


^ You have done it here.

blazeu · 2017-12-17T14:57:10Z

static/js/global.js

+    })
+}
+
+$(window).on('load', function() {


$(window).on('load') is here ^

blazeu · 2017-12-17T15:26:30Z

static/js/global.js

+  var nav = window.navigator
+  browserLocale =
+    nav.language.split('-')[0] || nav.language || nav.userLanguage || ''
+  localStorage.setItem('lang', browserLocale)


Only store if the user has specifically selected a language.

This adds i18n support for the app, being rendered in client-side so that translations can be loaded dynamically. Implemented language fallbacks for non-existing translations, plurals and dynamic placeholders for strings. Fixes coala#38

blazeu · 2017-12-17T15:53:29Z

ack 2899de9

done

gitmate-bot added process/pending review size/XXL area/i18n area/ui difficulty/medium initiatives/gci labels Dec 13, 2017

mooncos force-pushed the i18n-patch branch 2 times, most recently from c464ead to 148d879 Compare December 14, 2017 00:17

blazeu requested changes Dec 14, 2017

View reviewed changes

jayvdb requested changes Dec 14, 2017

View reviewed changes

mooncos force-pushed the i18n-patch branch from 148d879 to 8659cc6 Compare December 15, 2017 17:55

gitmate-bot added size/S and removed size/XXL labels Dec 15, 2017

mooncos force-pushed the i18n-patch branch from 8659cc6 to 0898f27 Compare December 15, 2017 18:11

gitmate-bot added process/approved and removed process/pending review labels Dec 15, 2017

mooncos force-pushed the i18n-patch branch from 0898f27 to 911e838 Compare December 15, 2017 18:15

gitmate-bot added process/pending review and removed process/approved labels Dec 15, 2017

jayvdb previously requested changes Dec 16, 2017

View reviewed changes

mooncos force-pushed the i18n-patch branch from 911e838 to 0159619 Compare December 16, 2017 16:08

mooncos force-pushed the i18n-patch branch from 0159619 to f587826 Compare December 17, 2017 13:26

blazeu requested changes Dec 17, 2017

View reviewed changes

blazeu reviewed Dec 17, 2017

View reviewed changes

static/css/main.css Outdated

.chooser {

margin-top: 0.5em;

display: flex;

Copy link

Member

blazeu Dec 17, 2017

Choose a reason for hiding this comment

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

I don't think you need flex

blazeu reviewed Dec 17, 2017

View reviewed changes

static/js/global.js

})

}

$(window).on('load', function() {

Copy link

Member

blazeu Dec 17, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

$(window).on('load') is here ^

mooncos force-pushed the i18n-patch branch from f587826 to 2e18327 Compare December 17, 2017 15:10

blazeu reviewed Dec 17, 2017

View reviewed changes

i18n: Add full i18n support

2899de9

This adds i18n support for the app, being rendered in client-side so that translations can be loaded dynamically. Implemented language fallbacks for non-existing translations, plurals and dynamic placeholders for strings. Fixes coala#38

mooncos force-pushed the i18n-patch branch from 2e18327 to 2899de9 Compare December 17, 2017 15:30

blazeu approved these changes Dec 17, 2017

View reviewed changes

gitmate-bot added process/approved and removed process/pending review labels Dec 17, 2017

blazeu merged commit 2899de9 into coala:master Dec 17, 2017

gitmate-bot added process/approved and removed process/approved labels Dec 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: Add full i18n support #84

i18n: Add full i18n support #84

mooncos commented Dec 13, 2017

blazeu left a comment •

edited

Loading

blazeu Dec 14, 2017

jayvdb Dec 14, 2017

jayvdb Dec 14, 2017

jayvdb Dec 14, 2017

blazeu Dec 14, 2017

jayvdb Dec 16, 2017

mooncos Dec 16, 2017

jayvdb Dec 16, 2017

jayvdb Dec 16, 2017

jayvdb Dec 16, 2017

jayvdb Dec 16, 2017

jayvdb Dec 16, 2017

jayvdb commented Dec 16, 2017

mooncos commented Dec 16, 2017

blazeu commented Dec 17, 2017

blazeu Dec 17, 2017

mooncos Dec 17, 2017

blazeu Dec 17, 2017

blazeu Dec 17, 2017

blazeu Dec 17, 2017

mooncos Dec 17, 2017

blazeu Dec 17, 2017 •

edited

Loading

mooncos Dec 17, 2017 •

edited

Loading

blazeu Dec 17, 2017

blazeu Dec 17, 2017

blazeu Dec 17, 2017 •

edited

Loading

blazeu Dec 17, 2017

mooncos Dec 17, 2017

blazeu commented Dec 17, 2017

		@@ -0,0 +1,24 @@
		var browser_locale = window.navigator.language.split('-')[0]

i18n: Add full i18n support #84

i18n: Add full i18n support #84

Conversation

mooncos commented Dec 13, 2017

blazeu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented Dec 16, 2017

mooncos commented Dec 16, 2017

blazeu commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

mooncos Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu commented Dec 17, 2017

blazeu left a comment •

edited

Loading

blazeu Dec 17, 2017 •

edited

Loading

mooncos Dec 17, 2017 •

edited

Loading

blazeu Dec 17, 2017 •

edited

Loading