Fully add multi-language capability to blip #473

coyotte508 · 2018-04-08T12:19:40Z

This is a follow-up to #468, with a lot more changes as every translatable string is flagged. Keys are used where html content is concerned, see 4e3b42d

This PR, along with the corresponding PRs in viz and tideline would allow multilanguage capabilities right now.

Procedure: - surround all text to be translated with __() - npm run update-translations will update the .po files in translations/ - Use appropriate software to add translations - npm run build-translations will transform the .po files into .json Then the webpack script takes care of generating a separate index.[xx].html and bundle.[xx].js for each language in dist. For now, the server only serves index.en.html

Set default language to French (pending a per-user setting) and add translations for the navbar

- Move the loading of translations out of dependencies/i18n - Change languages.json from [languageCode] to {languageCode: language} - Move the language setting out of bootstrap.js and into core/language - Add language selection in userprofile to change language - Language selection is only client-side, not yet stored server-side

If the user hasn't yet set a language in their settings, use the browser's language instead.

Not yet using react-i18next. i18next allows to keep things minimal, react-i18next would introduce decorators and extracting t from props. I am still using i18n-extract to extract translations, next step is using i18next-parser

- Sort translation keys in the JSON - Translate the front page - Remove unneeded dependencies - Fix stuff

The weekly/trends chart have also been updated from "MMM D" to "D MMM" in French

Translations are now extracted from blip's two submodules. That way they don't need the extra-tooling, only i18next as a dependency. Furthermore, they can prefix their translatable strings with 'viz|' or 'tideline|' to use a different namespace and separate their translations from blip's translation.

It gets confused by translate() calls in viz/tideline

clintonium-119 · 2018-04-09T19:13:37Z

Thanks @coyotte508

We're currently having our company-wide offsite this week. I should be available to dive into reviewing this (and the other corresponding PRs) next week.

clintonium-119 · 2018-04-20T19:36:12Z

@coyotte508 Just to clarify: This seems to fully replace the #468 PR, correct? I haven't gone through it in-depth yet, but at a glance it seems that this one encompasses everything from it.

coyotte508 · 2018-04-21T15:13:05Z

@cbwebdevelomnent yes, this is identical to #468, with more commits to add translations to every component. #468 is the proof of concept, this PR is a generalization to the whole of blip.

clintonium-119 · 2018-04-23T13:54:41Z

Code review(s) for Tidepool Web i18n

clintonium-119

Thanks for putting this all together! Looks good 👍

I do have a number of small changes requested, but overall a very clean PR.

The one main change I allude to in one of the review comments is that we'd like to have any translation-related UI changes, such as the language select on the user profile form, be enabled only by a feature flag (set via an envirionment variable).

Here are the local additions I made as a POC to illustrate the approach I recommend:

config.app.js

/* global __I18N_ENABLED__ */

module.exports {
  // ...
  I18N_ENABLED: booleanFromText(__I18N_ENABLED__, false),
};

config.webpack.js

var defineEnvPlugin = new webpack.DefinePlugin({
  // ...
  __I18N_ENABLED__: JSON.stringify(process.env.I18N_ENABLED || false),
});

webpack.config.js

var defineEnvPlugin = new webpack.DefinePlugin({
  // ...
  __I18N_ENABLED__: JSON.stringify(process.env.I18N_ENABLED || false),
});

Then, we can opt-in to show any translation-related UI elements via the config.app.js export

import config from './config';

config.I18N_ENABLED && showStuff();

clintonium-119 · 2018-04-24T13:34:58Z

app/components/chart/weekly.js

@@ -190,7 +191,7 @@ var Weekly = React.createClass({

  componentWillReceiveProps:function (nextProps) {
    if (this.props.loading && !nextProps.loading) {
-      this.refs.chart.rerenderChart();
+      this.refs.chart.getWrappedInstance().rerenderChart();


This errors out because the WeeklyChart class above is not wrapped with the translate HOC.

I'd suggest that we wrap the WeeklyChart class for consistency with the other views. We then need to ensure all calls to methods on this.refs.chart are prececeded by getWrappedInstance()

Currently, there are two places where this has not been done:

In componentDidMount

In handleClickOneDay

clintonium-119 · 2018-04-24T13:38:59Z

app/components/datasources/datasources.js

+    /* The following is for the translation extracter */
+    // t('year', {context: 'timeago'});t('years', {context: 'timeago'});
+    // t('month', {context: 'timeago'});t('months', {context: 'timeago'});
+    // t('week', {context: 'timeago'});t('minutes', {context: 'timeago'});


Should this be weeks here instead of minutes?

clintonium-119 · 2018-04-24T13:47:34Z

app/components/donateform/donateform.js

@@ -15,6 +15,8 @@

 import React, { Component } from 'react';
 import _ from 'lodash';
+import { translate, Trans } from 'react-i18next';
+import i18next from '../../core/language';


Unused import

clintonium-119 · 2018-04-24T14:25:08Z

app/pages/patientnew/patientnew.js

 import { bindActionCreators } from 'redux';

 import _ from 'lodash';
 import sundial from 'sundial';
 import { validateForm } from '../../core/validation';
+import i18next from '../../core/language';


Unused import

clintonium-119 · 2018-04-24T14:58:10Z

app/pages/userprofile/userprofile.js

+    return [
+      {name: 'fullName', label: t('Full name'), type: 'text'},
+      {name: 'username', label: t('Email'), type: 'email'},
+      {name: 'lang', label: t('Language'), type: 'select', items: [


Recommend that this be only displayed when enabled by a feature flag. Also, I ran this by our designer, and he'd prefer the language select as the last form input.

Here's what I've done locally as a POC:

const inputs = [ {name: 'fullName', label: t('Full name'), type: 'text'}, {name: 'username', label: t('Email'), type: 'email'}, {name: 'password', label: t('Password'), type: 'password'}, {name: 'passwordConfirm', label: t('Confirm password'), type: 'password'} ]; const langInput = { name: 'lang', label: t('Language'), type: 'select', items: [ {value: 'en', label: 'English'}, {value: 'fr', label: 'Français'}, ], placeholder: t('Select language...'), }; config.I18N_ENABLED && inputs.push(langInput); return inputs;

I'll add additional comments to the main thread re: using a feature flag to enable i18n.

clintonium-119 · 2018-04-24T15:31:17Z

package.json

@@ -83,11 +87,13 @@
  "devDependencies": {
    "babel-eslint": "6.1.2",
    "babel-plugin-rewire": "1.0.0-rc-6",
+    "babel-plugin-transform-decorators-legacy": "^1.3.4",


Version needs to be locked down

clintonium-119 · 2018-04-24T15:31:24Z

package.json

    "chai": "3.5.0",
    "chromedriver": "2.23.1",
    "eslint": "2.13.1",
    "eslint-plugin-react": "5.2.2",
    "gitbook-cli": "2.3.0",
+    "i18next-parser": "^1.0.0-beta9",


Version needs to be locked down

clintonium-119 · 2018-04-25T14:17:11Z

app/pages/userprofile/userprofile.js

+        fullName: formValues.fullName,
+        // This seems to be the best place to put language. Preferences and Settings
+        // both refer to a patient id, but language doesn't have anything to do with patients
+        language: formValues.lang


I've spoken with our backend engineer responsible for our data model, and he's requested that the appropriate language code be stored in the preferences object as displayLanguageCode

clintonium-119 · 2018-04-25T14:48:17Z

app/pages/app/app.js

@@ -418,6 +419,10 @@ export function mapStateToProps(state) {
      if (state.blip.loggedInUserId === state.blip.currentPatientInViewId) {
        userIsCurrentPatient = true;
      }
+
+      if (user && user.profile && user.profile.language) {
+        i18next.changeLanguage(user.profile.language);


This will need to be changed once the language key is stored in preferences.displayLanguageCode

clintonium-119 · 2018-04-25T14:55:31Z

app/pages/app/app.js

@@ -418,6 +419,10 @@ export function mapStateToProps(state) {
      if (state.blip.loggedInUserId === state.blip.currentPatientInViewId) {
        userIsCurrentPatient = true;
      }
+
+      if (user && user.profile && user.profile.language) {


Recommend using _.get here:

if (_.get(user, 'profile.language')) { ... }

clintonium-119 · 2018-04-25T15:17:36Z

I just changed the base to a new i18n branch (currently identical to master.

This will allow us to get these PR's merged a bit quicker, and will also allow us to create some staging builds on our side so our QA team can smoke test this before we merge into master.

coyotte508 · 2018-04-25T16:49:25Z

Thank you. I'm taking note of everything and hope to have the PR ready soon. Since the base has been locked down to the current state of master, I will also fix the current conflicts.

- I18N_ENABLED environment variable to enable language selection in the GUI - Bugs with `getWrappedInstance()` in the weekly chart - Move language to preferences.displayLanguageCode - Remove unused imports

…uage

coyotte508 · 2018-04-26T21:06:01Z

@cbwebdevelopment Changes were made to this PR, as well as tidepool-org/viz#102 and tidepool-org/tideline#331.

As language is now stored in preferences, the user needs to be fetched before generating the pdf, so that the web worker can have language information.

clintonium-119 · 2018-04-30T21:13:59Z

app/pages/patientdata/patientdata.js

    const hasDiabetesData = _.get(nextState, 'processedPatientData.diabetesData.length');

    // Ahead-Of-Time pdf generation for non-blocked print popup.
    // Whenever patientData is processed or the chartType changes, such as after a refresh
    // we check to see if we need to generate a new pdf to avoid stale data
-    if (patientDataProcessed && hasDiabetesData && !pdfGenerating && !pdfGenerated) {
+    if (userFetched && patientDataProcessed && hasDiabetesData && !pdfGenerating && !pdfGenerated) {


I'm curious. Were you running into any issues not having this check here? I can't recall anything from the user object being leveraged in the PDFs.

It's to load preferences and the language. When the language information was stored in profile.language there were no issues as it's loaded with the patient, but the preferences are loaded later and as such preferences.displayLanguageCode.

Without userFetched, the generated pdf on a page reload doesn't take into account the language, the user would have to go to their profile and back to the data.

clintonium-119 · 2018-04-30T21:26:07Z

It all looks good to me. I just have the one question about the need for the userFetched check as a requirement to PDF generation.

Also, it would be good to track that change in a separate PR. If you do move it to a separate PR, I can approve this one right away.

Nice work :) We really appreciate it.

coyotte508 · 2018-04-30T22:26:37Z

Thank you :)

I removed the last commit with userFetched, I'm going to make it in a separate PR.

clintonium-119 · 2018-04-30T22:32:15Z

Ahh makes sense. You can leave it in if you’d like. I didn’t realize it was related to the translation. Separate PR is fine as well if it’s already prepared. I’ll approve it at the same time.

…

On Apr 30, 2018, 6:26 PM -0400, coyotte508 ***@***.***>, wrote: Thank you :) I removed the last commit with userFetched, I'm going to make it in a separate PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

coyotte508 · 2018-04-30T22:35:47Z

Okay, added the commit back.

clintonium-119 · 2018-04-30T22:53:05Z

LGTM 👍

coyotte508 and others added 25 commits April 8, 2018 12:00

Add package-lock.json to .gitignore

d2a7111

Add a more dynamic way of serving translations

ac5c38f

Set default language to French (pending a per-user setting) and add translations for the navbar

Language is stored and preserved across logins #2

87af4cf

Use browser language if no language set

665b618

If the user hasn't yet set a language in their settings, use the browser's language instead.

Use i18next for translation rendering #2

da95783

Not yet using react-i18next. i18next allows to keep things minimal, react-i18next would introduce decorators and extracting t from props. I am still using i18n-extract to extract translations, next step is using i18next-parser

Remove server rendering code for translations

5dbb993

Use i18next-parser instead of i18n-extract #2

1dac810

Upgrade from i18next to react-i18next #2

a516bbc

Remove unnecessary dependency

d4b313a

Touching up #2

810c23b

- Sort translation keys in the JSON - Translate the front page - Remove unneeded dependencies - Fix stuff

Fix the tests

8becb91

Add basic test for languages

bb93919

Add date translation

490c1e5

The weekly/trends chart have also been updated from "MMM D" to "D MMM" in French

Remove decorator, replace with HOC

7c46485

Translate the rest of blip

fc43cc6

Superfluous t() removed

1488e94

Remove the reactNamespace option

bb98eab

It gets confused by translate() calls in viz/tideline

Add last getWrappedInstance() on a ref

df3ee2f

translate save message in User profile

cc79283

Fixed translations for getDefaultProps

203d080

Save language in local storage

2a4394b

Fix runtime error

6293c4c

This was referenced Apr 8, 2018

Localization tidepool-org/viz#102

Merged

Localization tidepool-org/tideline#331

Merged

Update i18next-parser and add keys to Trans component

4e3b42d

coyotte508 force-pushed the language branch from 6838af1 to 4e3b42d Compare April 9, 2018 13:22

clintonium-119 self-requested a review April 9, 2018 19:14

Fix combination of variables and <Trans> component

a81ae0c

coyotte508 force-pushed the language branch from f2b0303 to a81ae0c Compare April 10, 2018 08:44

clintonium-119 mentioned this pull request Apr 23, 2018

Add multi-language capability to blip with react-i18next #468

Closed

clintonium-119 requested changes Apr 25, 2018

View reviewed changes

clintonium-119 changed the base branch from master to i18n April 25, 2018 15:14

Lock down package versions

48566a5

Changes requested as per review

a1f2224

- I18N_ENABLED environment variable to enable language selection in the GUI - Bugs with `getWrappedInstance()` in the weekly chart - Move language to preferences.displayLanguageCode - Remove unused imports

coyotte508 force-pushed the language branch from 665b18b to a1f2224 Compare April 26, 2018 20:08

Merge branch 'i18n' of https://github.com/tidepool-org/blip into lang…

312aef9

…uage

coyotte508 force-pushed the language branch from d90be90 to 312aef9 Compare April 26, 2018 20:23

Fix tests

0895c88

coyotte508 force-pushed the language branch from 65e55ae to 0895c88 Compare April 26, 2018 21:03

Wait for user to be fetched before sending pdf request

2e1eb23

As language is now stored in preferences, the user needs to be fetched before generating the pdf, so that the web worker can have language information.

clintonium-119 reviewed Apr 30, 2018

View reviewed changes

coyotte508 force-pushed the language branch from 2e1eb23 to 0895c88 Compare April 30, 2018 22:25

coyotte508 mentioned this pull request Apr 30, 2018

Wait longer for pdf generation in order to load user language beforehand #477

Closed

clintonium-119 approved these changes Apr 30, 2018

View reviewed changes

clintonium-119 merged commit 1442d09 into tidepool-org:i18n May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully add multi-language capability to blip #473

Fully add multi-language capability to blip #473

coyotte508 commented Apr 8, 2018 •

edited

Loading

clintonium-119 commented Apr 9, 2018

clintonium-119 commented Apr 20, 2018

coyotte508 commented Apr 21, 2018

clintonium-119 commented Apr 23, 2018

clintonium-119 left a comment

clintonium-119 Apr 24, 2018

clintonium-119 Apr 24, 2018

clintonium-119 Apr 24, 2018

clintonium-119 Apr 24, 2018

clintonium-119 Apr 24, 2018

clintonium-119 Apr 24, 2018

clintonium-119 Apr 24, 2018

clintonium-119 Apr 25, 2018

clintonium-119 Apr 25, 2018

clintonium-119 Apr 25, 2018

clintonium-119 commented Apr 25, 2018

coyotte508 commented Apr 25, 2018

coyotte508 commented Apr 26, 2018 •

edited

Loading

clintonium-119 Apr 30, 2018

coyotte508 Apr 30, 2018 •

edited

Loading

clintonium-119 commented Apr 30, 2018

coyotte508 commented Apr 30, 2018

clintonium-119 commented Apr 30, 2018 via email

coyotte508 commented Apr 30, 2018 •

edited

Loading

clintonium-119 commented Apr 30, 2018

Fully add multi-language capability to blip #473

Fully add multi-language capability to blip #473

Conversation

coyotte508 commented Apr 8, 2018 • edited Loading

clintonium-119 commented Apr 9, 2018

clintonium-119 commented Apr 20, 2018

coyotte508 commented Apr 21, 2018

clintonium-119 commented Apr 23, 2018

clintonium-119 left a 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

clintonium-119 commented Apr 25, 2018

coyotte508 commented Apr 25, 2018

coyotte508 commented Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

coyotte508 Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

clintonium-119 commented Apr 30, 2018

coyotte508 commented Apr 30, 2018

clintonium-119 commented Apr 30, 2018 via email

coyotte508 commented Apr 30, 2018 • edited Loading

clintonium-119 commented Apr 30, 2018

coyotte508 commented Apr 8, 2018 •

edited

Loading

coyotte508 commented Apr 26, 2018 •

edited

Loading

coyotte508 Apr 30, 2018 •

edited

Loading

coyotte508 commented Apr 30, 2018 •

edited

Loading