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

Multi-language and French #344

Closed
wants to merge 38 commits into from
Closed

Conversation

nikkozzblu
Copy link
Contributor

Not ready for production,

Having some problems with the panel-left new styles (Css not being updated ?!). Also notice wind/solar icons and scale not being displayed.

try it on http://localhost:8000/?lang=fr

You mainly find 2 files by language extension:
/locales/$lang.json
which contain the static translations (processed server side)

/app/config/lang.json
which contains the translations of the label that are filled dynamically on JS, append new language here

@corradio
Copy link
Member

corradio commented Feb 6, 2017

I'm renaming to WIP - .... to make it clear it's not ready for review

@corradio corradio changed the title Multi-language and French WIP - Multi-language and French Feb 6, 2017
@nikkozzblu
Copy link
Contributor Author

OK now should be compiling, I don't want to spend too much time on integrating latest changes like time series since it seems that it is changing fast in the coming days,...

Hope the latest merging were correct
Feel free to review when you get a chance

@nikkozzblu nikkozzblu changed the title WIP - Multi-language and French Multi-language and French Feb 6, 2017
@corradio corradio self-requested a review February 6, 2017 19:28
@corradio corradio self-assigned this Feb 6, 2017
@corradio
Copy link
Member

corradio commented Feb 6, 2017

Thanks! I'll review asap but we will need to merge master anyway..
There's no major changes upcoming so this will be the last one.

Copy link
Member

@corradio corradio left a comment

Choose a reason for hiding this comment

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

Pretty cool!
The merge will be needed sorry.. If I had known I would have waited :(
I suggest:
1/ fix the indentations to have a clean diff
2/ do a merge master

@@ -33,6 +33,9 @@ function CountryTable(selector, co2Color, modeColor, modeOrder) {
this.SCALE_TICKS = 4;
this.TRANSITION_DURATION = 250;


this.LANG = lang;
console.log('a',this.LANG);
Copy link
Member

Choose a reason for hiding this comment

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

Not needed

@@ -368,7 +371,7 @@ CountryTable.prototype.resize = function() {

CountryTable.prototype.data = function(arg) {
var that = this;

console.log(this._data);
Copy link
Member

Choose a reason for hiding this comment

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

not needed

<div class="country-picker-container small-screen-visible" style="margin-left: 20px">

<div class="country-table-initial-text">
<p class="small-screen-visible" style="font-style: italic">
Copy link
Member

Choose a reason for hiding this comment

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

Additional indentation creates a lot of diff

<svg class="co2-colorbar colorbar" style="width: 100%"></svg>
</p>
<p>
<p>
Copy link
Member

Choose a reason for hiding this comment

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

Indent problem

@@ -35,8 +36,10 @@ var selectedCountryCode;
var forceRemoteEndpoint = false;
var customDate;
var timelineEnabled = false;
var reqLang = 'en';
Copy link
Member

Choose a reason for hiding this comment

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

how is the language changed except by querystring argument? Shouldn't it be detected automatically? Maybe you can use ~L134 where we set the moment locale?

// Set proper locale
var locale = window.navigator.userLanguage || window.navigator.language;
moment.locale(locale);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The translation is a mix of server (through i18n) and client, so far i had no luck to get the server understanding same locale as client but i ll have another try since it shoudl be the defaut behaviour of the i18n library. As matter of consistency i therefore also use the query string on client.

Copy link
Member

Choose a reason for hiding this comment

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

ok can you then also update the locale used by moment for consistency? Should be around L134.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was what I did originally but because my french locale was not understood by server.js, it results in some mix between English text/French "source names"...
the server should be able to understand fr from i18n spec, not sure what is going wrong here.
Anyway I thought the query string could be a good intermediate solution for first release before we get the text actually read and corrected by external reviewer

@corradio corradio mentioned this pull request Feb 6, 2017
@corradio
Copy link
Member

corradio commented Feb 7, 2017

@nikkozzblu please let me know when to review!

@nikkozzblu
Copy link
Contributor Author

Should be alright now, (hope so, I was almost able to run a stable development environment but just lost my VM connection as I change Wi-Fi network... :()

@nikkozzblu
Copy link
Contributor Author

Should be alright now, (hope so, I was almost able to run a stable development environment but just lost my VM connection as I change Wi-Fi network... :()

Copy link
Member

@corradio corradio left a comment

Choose a reason for hiding this comment

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

Almost there.

We need to support i10n for data shared on Facebook too. See https://developers.facebook.com/docs/opengraph/guides/internationalization

  • On the / route, set locale based on the fb_locale query string (values received comply to https://developers.facebook.com/docs/internationalization/#locales). We should also add a locale or lang querystring for ourselves.
  • For each language supported, add a meta property <meta property="og:locale:alternate" content="es_ES" />
  • For the current language used, set <meta property="og:locale" content="en_US" />

I'll also verify all translations.

@@ -275,4 +275,4 @@ div.fb-share-button, div.fb-like, div.fb-follow, iframe.twitter-share-button, if

.time-travel {
display: none;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This file shouldn't change

web/server.js Outdated
//multi-language
i18n.configure({
// where to store json files - defaults to './locales' relative to modules directory
locales:['en', 'fr'],
Copy link
Member

Choose a reason for hiding this comment

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

Style: Missing space after :

@@ -112,8 +110,8 @@
<input type="text" class="flatpickr" />
</div>
</div>
<p>
<span id="country-table-back-button" style="display: none"><a><< back</a></span>
<p>
Copy link
Member

Choose a reason for hiding this comment

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

Style: Indentation

<svg class="co2-colorbar colorbar" style="width: 100%"></svg>
</p>
<p>
Copy link
Member

Choose a reason for hiding this comment

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

Style: indentation

<svg class="solar-colorbar colorbar" style="width: 100%; display:none"></svg>
</p>
</div>
</div>
<div>
<hr />
Found bugs or have ideas? Report them <a href="https://github.com/corradio/electricitymap/issues/new" target="_blank">here</a>.<br />
Copy link
Member

Choose a reason for hiding this comment

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

Style: indentation. Also seems like order of two lines has been changed.

var currentMoment;


Copy link
Member

Choose a reason for hiding this comment

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

Extra line not needed

@@ -82,6 +85,8 @@ args.forEach(function(arg) {
} else if (kv[0] == 'countryCode') {
selectedCountryCode = kv[1];
replaceHistoryState('countryCode', selectedCountryCode);
} else if(kv[0] == 'lang'){
reqLang = kv[1];
Copy link
Member

Choose a reason for hiding this comment

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

  • Style: indent

@@ -82,6 +85,8 @@ args.forEach(function(arg) {
} else if (kv[0] == 'countryCode') {
selectedCountryCode = kv[1];
replaceHistoryState('countryCode', selectedCountryCode);
} else if(kv[0] == 'lang'){
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually not in favor of using a querystring argument because the URL is used when sharing to social media (and we don't want to url to be part of that).
I suggest instead that the node server adds a global variable to the template, thus defining the locale.
Then you don't need to read it here.

@@ -15,7 +15,7 @@ function getConsumption(country) {
}

// ** Country table
exports.setupCountryTable = function (countryTable, countries, co2Colorbar, co2color) {
exports.setupCountryTable = function (countryTable, countries, co2Colorbar, co2color,lang) {
Copy link
Member

Choose a reason for hiding this comment

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

Style: missing space after ,

web/package.json Outdated
@@ -17,7 +17,8 @@
"mongodb": "^2.2.9",
"opbeat": "^3.21.0",
"snappy": "^5.0.5",
"topojson": "^2.2.0"
"topojson": "^2.2.0",
"i18n":"^0.8.3"
Copy link
Member

Choose a reason for hiding this comment

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

Style: Indentation error

@corradio
Copy link
Member

corradio commented Feb 7, 2017

Btw I added a section for global variables coming from the server:
a0a35ef#diff-a344ab15f45451bd3fe93462dc8325d1R29

@nikkozzblu nikkozzblu closed this Feb 7, 2017
corradio pushed a commit that referenced this pull request Feb 7, 2017
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.

2 participants