Add i18n library to Linkerd dashboard #4803

scottcarol · 2020-07-28T00:23:59Z

Part of #3651

This PR adds the LinguiJS project to the Linkerd dashboard for i18n and translation. This is an open source project that is well documented and updated, with an active community. After evaluating several libraries this seemed like a lightweight option with easy CLI tools, making it easy for other contributors to learn and use.

This PR ONLY changes 2 React components. The purpose is for reviewers to evaluate the use of the library, adding a language, editing translations, compiling, etc. After this approach is validated a second PR will go up which adds translations for all of the remaining components.

How to test this PR

Viewing the dashboard

Check out this branch and run bin/web dev or bin/web run.
With your browser language set to en open http://localhost:7777/namespaces/linkerd/replicationcontrollers
If you have the standard Linkerd setup you should see:
Change your browser language to es. You should see:

Adding a translation

Open Namespace.jsx in your code editor.
Apply this diff. Note importing Trans and wrapping text in it.

diff --git a/web/app/js/components/Namespace.jsx b/web/app/js/components/Namespace.jsx
index 65f32e77..c33a33e8 100644
--- a/web/app/js/components/Namespace.jsx
+++ b/web/app/js/components/Namespace.jsx
@@ -7,6 +7,7 @@ import NetworkGraph from './NetworkGraph.jsx';
 import PropTypes from 'prop-types';
 import React from 'react';
 import Spinner from './util/Spinner.jsx';
+import { Trans } from '@lingui/macro';
 import _filter from 'lodash/filter';
 import _get from 'lodash/get';
 import _isEmpty from 'lodash/isEmpty';
@@ -147,7 +148,7 @@ class Namespaces extends React.Component {
         {!error ? null : <ErrorBanner message={error} />}
         {!loaded ? <Spinner /> : (
           <div>
-            {noMetrics ? <div>No resources detected.</div> : null}
+            {noMetrics ? <div><Trans>No resources detected.</Trans></div> : null}
             {
               _isEmpty(deploymentsWithMetrics) ? null :
               <NetworkGraph namespace={ns} deployments={metrics.deployment} />

From web/app run yarn extract
It should indicate that you have 1 missing translation in each file
Open locales/es/messages.json and locales/en/messages.json and fill in the blanks.
Run yarn extract again. It should say you have 0 missing translations.
Run yarn compile.
Load http://localhost:7777/namespaces/blabhablhab from an en browser and an es browser. You should see the translations you put in. If your browser is set to a different language, only the English key will show.

Adding a locale

From web/app run yarn add-locale fr then yarn extract
Open locales/fr/messages.json and add sample values
Add import catalogFr from './locales/fr/messages.js’; to index.js
Run yarn compile
Load dashboard with your browser set to fr

Signed-off-by: Carol Scott carol@buoyant.io

.gitignore

bin/web

scottcarol · 2020-07-28T00:25:16Z

web/app/js/components/EmptyCard.jsx

  return (
    <Card className={classes.card} elevation={3}>
      <CardContent>
        <Typography>
-          {content}
+          <Trans>No data to display</Trans>


This is an example of translating a static text string.

Also, we were never passing in a prop content to this component, so we could remove the propTypes checks.

scottcarol · 2020-07-28T00:25:49Z

web/app/js/components/util/CopyUtils.jsx

+
+  return (
+    <Typography variant="body2">
+      <Trans>


This is an example of translating a block of text containing a dynamic text value.

web/app/js/index.js

alpeb

Thanks @scottcarol , the explained workflow feels pretty smooth 💃 (you only missed mentioning under the "adding locale section" adding the new locale into the catalogs const).

One very specific edge case I tested was "generic locale fallback" support, that helps overriding specific translations that differ across countries using the same language (that I also mentioned in the previous PR). So I added one catalog for es and another for es-CO; by the way this is the right format that took me a bit to figure:

const catalogs = { en: catalogEn, es: catalogEs, "es-CO": catalogEsCo, fr: catalogFr };

If I browsed the site with a "es-CO" locale it will rightly show the translations. If a message isn't translated it will fallback to the English word, but I was hoping it would fallback to the generic es translation. Maybe that's just a lingui limitation? Anyways, not a deal breaker if so.

alpeb · 2020-07-28T21:30:57Z

web/app/js/index.js

@@ -72,9 +75,13 @@ class App extends React.Component {
  }

  render() {
+    const catalogs = { en: catalogEn, es: catalogEs };


Does this mean every translation will get loaded regardless of the current user language? If so, probably not a big deal, but would be better if they could be loaded dynamically...

Good point, I updated it so that we are only passing the necessary catalog into I18nProvider.

I tried taking it one step further and dynamically loading the specific catalog we needed into index.js:

const selectedCatalog = require(`./locales/${selectedLocale}/messages.js`);

but I hit the no-dynamic-require es-lint rule, which seems to be best practices, so I am continuing to load all of the possible catalogs into this file.

alpeb · 2020-07-28T21:39:19Z

web/app/package.json

+    "add-locale": "lingui add-locale",
+    "extract": "lingui extract",
+    "compile": "lingui compile"


These aliases can be less expressive than the original command. yarn compile in particular sounds like it would do more than compile locales 😉

Yes, that's a good point. To be more explicit, I updated the commands so they include lingui.

In that case the scripts section is no longer necessary, is it? Or are you leaving that there just to document the available commands?

You're totally right, it's not needed now. Just removed, and the BUILD.md should be the file with the documentation of commands. I can add documentation in the follow-up PR I will make after this.

scottcarol · 2020-07-29T06:05:40Z

@alpeb Thanks for taking a look!

Based on your feedback around generic locale fallback support, I took a look in the js-lingui repo and they recommend using the locales-detector library, so I added this.

If my browser locales are es-AR, fr-CA, we create an array of languages: ['es-AR', 'es', 'fr-CA', 'fr'] and return the first match to an existing catalog (and default to the en catalog if there are no matches).

If this updated branch makes sense to you, I'll pull out the logic into its own util file and add tests.

alpeb · 2020-07-29T15:00:03Z

Nice, this fixes the case when say the browser uses es-AR and if that locale is missing it falls back to es 👍

Do you know if there's a chance to have this fallback mechanism operate at the single phrase level?
E.g. I have a complete catalog locales/es/messages.js with all the entries for standard Spanish, and I also have locales/es_AR/messages.js but that only contains overrides for phrases that differ from the standard Spanish. So if an entry is not there, it should fallback to the entry from the es locale.
Maybe if lingui doesn't support that, the right workflow in that case could be having users copy all the entries from es and overwrite what they need in es-AR...

scottcarol · 2020-07-29T15:20:37Z

Good question @alpeb:

Do you know if there's a chance to have this fallback mechanism operate at the single phrase level?

When we run lingui add-locale my-LOCALE and then lingui extract it creates a new JSON file with all of the existing keys and empty values. If we want to use the lingui extract command to call out anything we need to fix/add, we will need to have complete keys/values for each translation file. Otherwise any empty values would be listed as "missing" in the command output. I'm thinking there is a benefit for each translation file to stand independently, since it might be hard to communicate which values are actually missing and which values are intentionally blank, but I'll look back at the docs.

scottcarol · 2020-07-29T16:30:16Z

@alpeb I can't immediately find best practices on individual default fallback values, but if it's ok with you, we could merge this PR, in a follow-up PR add translations for es and en, and then decide on our process for adding locale-SPECIFIC formats.

alpeb

@alpeb I can't immediately find best practices on individual default fallback values, but if it's ok with you, we could merge this PR, in a follow-up PR add translations for es and en, and then decide on our process for adding locale-SPECIFIC formats.

Yeah definitely! I was you wondering about that since I remembering doing that for some project a thousand years ago, but this is shippable as is 👍 🥇

grampelberg

LGTM, we probably want to use keys instead of the strings themselves as keys but that's an eventual improvement.

Add linguijs library

dad60af

scottcarol requested a review from a team as a code owner July 28, 2020 00:23

scottcarol self-assigned this Jul 28, 2020

scottcarol commented Jul 28, 2020

View reviewed changes

.gitignore Show resolved Hide resolved

scottcarol commented Jul 28, 2020

View reviewed changes

bin/web Outdated Show resolved Hide resolved

scottcarol commented Jul 28, 2020

View reviewed changes

web/app/js/index.js Outdated Show resolved Hide resolved

scottcarol requested review from alpeb and grampelberg July 28, 2020 00:26

alpeb reviewed Jul 28, 2020

View reviewed changes

alejandro feedback

f9f1d7f

Carol Scott added 2 commits July 29, 2020 07:47

remove yarn scripts

27aa8b6

updating yarn.lock

b64a69e

remove 'run'

d94892d

alpeb approved these changes Jul 29, 2020

View reviewed changes

grampelberg approved these changes Jul 30, 2020

View reviewed changes

scottcarol merged commit eec8905 into main Jul 30, 2020

scottcarol deleted the cscott/linguijs branch July 30, 2020 16:10

alpeb mentioned this pull request Jul 30, 2020

Change notes for edge-20.7.5 #4816

Merged

scottcarol mentioned this pull request Aug 12, 2020

Add es translations and docs instructions to dashboard #4866

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add i18n library to Linkerd dashboard #4803

Add i18n library to Linkerd dashboard #4803

scottcarol commented Jul 28, 2020

scottcarol Jul 28, 2020

scottcarol Jul 28, 2020

alpeb left a comment

alpeb Jul 28, 2020

scottcarol Jul 29, 2020

alpeb Jul 28, 2020

scottcarol Jul 29, 2020

alpeb Jul 29, 2020

scottcarol Jul 29, 2020

scottcarol commented Jul 29, 2020 •

edited

Loading

alpeb commented Jul 29, 2020

scottcarol commented Jul 29, 2020

scottcarol commented Jul 29, 2020

alpeb left a comment

grampelberg left a comment

Add i18n library to Linkerd dashboard #4803

Add i18n library to Linkerd dashboard #4803

Conversation

scottcarol commented Jul 28, 2020

How to test this PR

Viewing the dashboard

Adding a translation

Adding a locale

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpeb 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

scottcarol commented Jul 29, 2020 • edited Loading

alpeb commented Jul 29, 2020

scottcarol commented Jul 29, 2020

scottcarol commented Jul 29, 2020

alpeb left a comment

Choose a reason for hiding this comment

grampelberg left a comment

Choose a reason for hiding this comment

scottcarol commented Jul 29, 2020 •

edited

Loading