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

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

Closed
wants to merge 17 commits into from

Conversation

coyotte508
Copy link
Contributor

@coyotte508 coyotte508 commented Mar 22, 2018

Related issue: #449

As a proof of concept, the front page, the navbar and the account settings page have translations added for French. Language can be changed in the account settings page.

The workflow changes are described below:

Extracting translation keys from code

Just run npm run update-translations. It uses i18next-parser under the hood.

The json files localted in locales/xx are updated.

Translating components

There are two steps: wrapping the component with translate(namespace?, options?)() from react-i18next, and extracting t from this.props.

When using React.createClass:

import React from 'react';
import { translate } from 'react-i18next';

const Component = translate()(React.createClass({
  // ...
  render: function() {
    // t will translate based on the current language and  namespace given to translate(), 
    // or the default namespace if no namespace was given to translate()
    const { t } = this.props;
    return <span>{t('Hello World')}</span>;
 }
}));

When using the ES6 way of creating components, translate can be used as a decorator:

@translate()
class Component extends React.Component {
  // ...
  render() {
    const { t } = this.props;
    return <span>{t('Hello World')}</span>;
  }
}

The default namespace is unchanged and is 'translation'.

An alternative to using the translate decorator on the component is wrapping the rendered content with the I18n component, as seen here.

Warning: When trying to access member function of decorated components from the outside, you will need to 'unwrap' the component.

For that you need to first add withRef as an parameter to translate:

var Navbar = translate('translation', {withRef: true})(React.createClass({
  hideDropdown: function()  {...}
}

With that is it possible to access hideDropdown from app.js:

class App extends React.Component {
  //...

  hideNavbarDropdown() {
    var navbar = this.refs.navbar;

    if (navbar) {
      // Here, .getWrappedInstance() is used to access the component
      navbar.getWrappedInstance().hideDropdown();
    }
  }
}

Interpolation, templated strings

Something like:

const message = `Hello, ${name}!`;
const countMessage = `There are approximately ${Math.round(numHours)} hours left`;

Would become:

const message = t('Hello, {{name}}!', {name});
const countMessage = t('There are approximately {{count}} hours left', {count: Math.round(numHours)});

Html mixed with text

Something like:

<div>
  Hello, {name}. You have <strong onClick={handleClick}>{messages.length}</strong> notifications.
</div>

Would become:

import { Trans } from 'react-i18next';

<Trans i18nkey='html.hello-notification'>
  Hello, {{name}}. You have <strong onClick={handleClick}>{{count: messages.length}}</strong> notifications.
</Trans>

The key as interpreted by i18next would be html.hello-noticiation, and the default value would be this: Hello, <1>{{name}}</1>. You have <3>{{count: messages.length}}</3> notifications.. As such, the content of the html doesn't appear in the translation and is not editable by the translator. Moreover, the html can be changed without changing the key.

Translating submodules

@tidepool/viz and tideline can add tranlations by adding i18next as a dependency, and using i18next.t(), blip is already configured to extract translatable strings from them.

They can prefix their translatable strings with tideline| or viz| to use separate namespaces from blip.

Considerations

It is possible to implement lazy loading of translations with i18next-xhr-backend with a change in webpack's config file, however locize is to be used in the future, with translations stored in the cloud.

Only a small part of the application has been translated. The idea is, if this PR is accepted and multi-language functionnality added to tidepool, to make a pull request to translate every component, maybe another to use keys as translation strings, and another to translate viz and tideline. Each of these further PRs can be subject to debate (amount of code changed, implementation, possible undetected errors when mass-updating the app...) and this is why I feel that an incremental process is better, rather than a monolithic PR with massive code changes.

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
@pazaan pazaan requested a review from krystophv March 23, 2018 07:09
@coyotte508 coyotte508 force-pushed the master branch 2 times, most recently from 7fc2bd6 to 162806f Compare March 23, 2018 13:25
The weekly/trends chart have also been updated from "MMM D" to "D MMM" in French
@coyotte508 coyotte508 force-pushed the master branch 2 times, most recently from 0f366c1 to 1134b52 Compare March 23, 2018 15:52
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.
@@ -35,6 +36,7 @@ const TrendsContainer = viz.containers.TrendsContainer;
const reshapeBgClassesToBgBounds = viz.utils.reshapeBgClassesToBgBounds;
const Loader = viz.components.Loader;

@translate()
Copy link
Member

Choose a reason for hiding this comment

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

Since we currently don't have any decorator patterns in the codebase, I'd be hesitant to add it right now. I'm not opposed to the idea, but perhaps I'd like @cbwebdevelopment's opinion as well.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I actually do like decorator patterns (I've used them on past projects), but for consistency with the rest of the codebase at this point, I think we should hold off on introducing them, and use the HOC approach used everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR by removing the decorator pattern. I also marked the rest of blip for translation (all tests passing) in the language branch of my fork, ready to make another PR.

@clintonium-119
Copy link
Member

Closing this proof-of-concept PR, which is superseded by #473
see #473 (comment)

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