I18n for static strings in UI #403

vencax · 2017-05-04T17:24:40Z

Hi, because not all of our editors and contributors speak english I decided to make an i18n for netlify-cms.
The idea is simple: all strings direct through __ function which takes i18n strings object and simply pass given string through that object.
I have included some tools to extract the strings automatically for easier i18n of whole app.

I am looking forward for some feedback alredy :)

erquhart · 2017-05-04T18:01:29Z

Hey @vencax, thanks for this! Discussion of 18n for the cms has increased as of late, so it's nice to see some action :)

I have two thoughts on direction here:

We'll want to match keys (rather than entire strings) when storing translations
There are a couple of well supported solutions out there, namely Yahoo's react-intl and Airbnb's polyglot.js. We'll probably want to leverage one of them.

An implementation along those lines would get us pretty close to something we can merge. Let me know what you think!

vencax · 2017-05-09T07:17:30Z

Yes, why not, but I am not sure if I will find time to do in in next week, since I am going to travel bit. Let's see :)

vencax · 2017-05-18T19:47:45Z

Ok, I would use polyglot ...

erquhart · 2017-05-18T20:38:26Z

Polyglot is certainly simpler, agreed.

vencax · 2017-05-19T09:16:24Z

Ok, I have reworked the i18n solution with polyglot. Let me know if this is acceptable :)

Benaiah

Thanks for the contribution! Needs some work, but it's a solid start. The following are some of the particularly important changes:

We shouldn't be loading JS dependencies or translation files from third-party URLs at runtime. polyglot should be a dependency tracked in package.json and included in the cms.js bundle.
polyglot should not be a window global, and should not be set up in a separate <script> tag.
We need to organize and plan out our translation keys so it doesn't get unwieldy as the number of translated strings grows.

Benaiah · 2017-06-23T21:29:11Z

example/index.html

@@ -6,6 +6,7 @@
  <title>This is an example</title>

  <link href='https://fonts.googleapis.com/css?family=Roboto:300,400,500' rel='stylesheet' type='text/css'>
+  <script src="https://rawgit.com/airbnb/polyglot.js/master/build/polyglot.js" type="text/javascript"></script>


This should be an NPM dependency (https://www.npmjs.com/package/node-polyglot), not a script tag referencing an external resource.

Because this project uses npm for dependency management.

Polyglot is NODE library. There are builds for frontend that ARE loaded as html scripts like I have done. If you install it as npm depend, it will give you error because it rely on node fs module.
Other argument is that not everything should be bundled. For instance React, lodash and some more libs can (and actually should) be externalized to make to bundle size smaller ...

@vencax it isn't just about bundle size, it's about automated dependency management. This project uses npm for that. Webpack should handle importing polyglot just fine.

I know what you mean, but the version installed by npm is NOT frontend version of polyglot. Try it, and you'll see ...

Benaiah · 2017-06-23T21:29:35Z

example/index.html

@@ -76,6 +77,9 @@
 </head>
 <body>

+  <script>
+    var polyglot = new Polyglot();


This should go in the CMS source, not as a script tag. We also shouldn't be using globals except to provide a hook point for end users (i.e., not for internal CMS stuff).

Benaiah · 2017-06-23T21:30:16Z

extract.js

+  marker: 'polyglot.t',
+});
+
+function _mergeMissing(transls, missing, makeVal) {


There's no need for the underscore prefix, or for abbreviating translations as transls.

Benaiah · 2017-06-23T21:30:42Z

extract.js

+  });
+}
+
+function _writeTransls(file, transls) {


There's no need for the underscore prefix, or for abbreviating translations as transls.

Benaiah · 2017-06-23T21:31:07Z

extract.js

+// check english trans first, they MUST be complete !!!
+const enFile = 'en.json';
+const enFilePath = path.join(i18nFolder, enFile);
+const enTransls = JSON.parse(fs.readFileSync(enFilePath));


Use asynchronous IO functions, and be sure to handle errors when doing IO. Don't abbreviate translations.

the same as above

Benaiah · 2017-06-23T21:44:51Z

docs/contributor-guide.md

+
+## i18n
+
+to extract/update i18n string run:


This should be expanded, it's quite unclear. It should also be correctly capitalized.

Benaiah · 2017-06-23T21:51:03Z

extract.js

@@ -0,0 +1,59 @@
+const i18n = require('i18n-extract');


This file should be moved to scripts, set up to use webpack (similarly to scripts/autoconfigure.collection.js), and these require calls should be changed to import ... from....

Benaiah · 2017-06-23T21:58:27Z

extract.js

+
+const errors = []
+
+fs.readdirSync(i18nFolder).forEach(file => {


Use asynchronous IO functions, and be sure to handle errors when doing IO.

the same as above

Benaiah · 2017-06-23T21:58:45Z

extract.js

+    return
+  }
+  const filePath = path.join(i18nFolder, file);
+  const transls = JSON.parse(fs.readFileSync(filePath));


Don't abbreviate translations. Use asynchronous IO actions, and be sure to handle errors when doing IO. (this one is particularly important - as written, the loop will wait for each file to be read in turn, whereas the equivalent asynchronous code could kick off all the file read promises in parallel).

Benaiah · 2017-06-23T21:59:06Z

extract.js

+
+function _writeTransls(file, transls) {
+  const sorted = _.keyArrange(transls)
+  fs.writeFileSync(file, JSON.stringify(sorted, {}, 2));


Use asynchronous IO actions, and be sure to handle errors when doing IO. Take a look at util.promisify (we may need a polyfill for older Node versions), which lets you use a promise-based interface to the Node stdlib.

it works without async IO, so no need to waste time with syntactic sugar, sorry :)

I'm not talking about asynchronous/await syntax sugar, I'm talking about using fs.writeFile instead of fs.writeFileSync.

vencax

some of the requested changes solved. Others left with comment below. And the unrelated newlines, really showstopper?

erquhart · 2017-07-21T18:33:49Z

src/actions/config.js

@@ -82,6 +83,27 @@ export function configDidLoad(config) {
  };
 }

+const DEFAULT_I18N_URL = 'https://raw.githubusercontent.com/netlify/netlify-cms/master/src';


As I mentioned in another comment, we can allow translation objects to be loaded in through the registry and forego any fetching while allowing translation to be independent of cms releases. I believe that would address concerns on both sides, let me know if you think otherwise.

The registry methods are added to the window object, source is here: https://github.com/netlify/netlify-cms/blob/1d08f1a33b9562e60145dc9704eb6379f64df044/src/lib/registry.js

Benaiah · 2017-07-21T20:32:52Z

@vencax the unrelated whitespace changes should be fixed in order to keep the history clean and usable. If you'd prefer l can edit them out and push the commit to the PR.

erquhart · 2017-07-22T02:34:59Z

@vencax just to reiterate, awesome work on this - we're looking forward to getting it merged. Let me know what you think about using the registry for i18n string objects.

@Benaiah can you go ahead and push a commit for removing whitespace changes if there are too many? Also, let me know if you're up for reviewing the naming scheme and providing some standardization so it matches with the general nomenclature elsewhere in the repo.

vencax · 2017-07-22T06:41:42Z

I still don't see advantage of using registry. The i18n strings are JSONs so they need to be loaded by request (ok, they can be loaded by webpack json-loader but it is IMHO much much much easier and removes the dependency on rebundlling to use the request). So the question is where to perform the request and the actual language init. NOT how to do it. Or am I missing something else?

erquhart · 2017-07-24T15:48:33Z

@vencax here's a simplistic breakdown of what I'd expect the Polyglot implementation to look like:

In the CMS source:

Load polyglot as an npm dependency, and import it where needed - Webpack should take care of browserifying
Provide a registry method that accepts an object, or maybe optionally an Immutable Map or string (string is expected to be JSON)

In the CMS integration page (the user's index.html):

Get the translation however you want, maybe via unpkg.com, or even create it from scratch.
Pass the translation object in to the i18n registry method.

I believe this simplifies things a great deal. Loading json really isn't an issue, just a string to parse.

Thoughts?

vencax · 2017-07-24T17:02:39Z

Ok,
@import: since the NPM version has broken browser support I have done compromise: usage of webpack, but the actual polyglot is loaded from CDN. Later, after browser support will be fixed (or somebody finds out way how to do it), npm dependency can be added and CDN version can be omited.
@i18n string loading: I still don't see any advantage by using registry :(

vencax · 2017-07-24T18:10:30Z

Ok, I have used webpack json-loader for load default (english strings). And during config phase if there is translations_url present in config, request is sent to retrieve them and if it is successful polyglot transls are replaced.
I think this is reasonable compromise and the solution is noninvasive.

erquhart · 2017-07-24T18:16:20Z

@vencax can you join us on Gitter to discuss?

https://gitter.im/netlify/netlifycms

It's just occurring to me that this setup requires a build step on the developer's side, and I don't believe that's necessary - we can probably cut out static analysis entirely and provide a simpler integration. A developer should only need to indicate a language in the config to use this, and those wanting something more advance can set it up as they see fit.

The other decision to make here is where strings are stored. I was assuming based on your earlier statements that you wanted them outside the CMS, which could work, but I now see that you're storing them within the CMS. If we store them within the CMS, then updating translations will be dependent on releases. I think this is fine since we can release such changes pretty rapidly. If we want to allow folks to override the built in translations, that can happen through the registry, which is the single point of extension for all aspects of the CMS.

erquhart · 2017-07-24T19:11:12Z

@vencax just took another look - it seems the user has to load Polyglot in their index.html or else the CMS won't load (you'll get errors when new Polyglot(...) runs, for example). With this PR introducing Polyglot as a requirement, we are going to need to load it through npm before we can merge. If you prefer not to dig into that, we can leave this until someone else has time.

vencax · 2017-07-24T21:37:20Z

NPM version used (I have originally used a wrong lib named the same :/ ).
Loading via webpack. Cheers! :)

erquhart · 2017-09-02T16:03:17Z

Remaining work to get this across the finish line:

rebase and resolve merge conflicts
abstract the i18n interface so that polyglot could be swapped out without breaking the API
expose i18n to widget authors, so that they can provide an object of translation objects with the widget

verythorough · 2017-12-30T06:50:57Z

Deploy preview ready!

Built with commit 77bb466

https://deploy-preview-403--cms-demo.netlify.com

verythorough · 2017-12-30T06:51:30Z

Deploy preview ready!

Built with commit 77bb466

https://deploy-preview-403--netlify-cms-www.netlify.com

vencax · 2017-12-30T11:11:51Z

Hi, I have created another PR (#974) so this could be closed.

vencax added 4 commits April 27, 2017 07:45

i18n vol.1

f7f46cd

extract npm script for extract i18n string

905126b

__ impl

6f08d7d

cs strings

4f48dba

vencax added 3 commits May 19, 2017 10:43

transls made by polyglot

f2b1783

forgotten imports

cec6835

polyglot init

86018b9

sorting transl strings

5f80a81

Benaiah suggested changes Jun 23, 2017

View reviewed changes

erquhart changed the title ~~I18n~~ I18n for static strings in UI Jun 26, 2017

vencax added 3 commits July 21, 2017 08:32

abbreviations removed, prefix _ removed

659f273

i18n url customisable, abbreviations removed

2a0cfe8

guide update

830d3f8

vencax commented Jul 21, 2017

View reviewed changes

erquhart suggested changes Jul 21, 2017

View reviewed changes

unrelated newlines removed

657ce96

polyglot by import (externalized)

cdf42b0

default english translations loaded by json-loader

db77f21

vencax added 3 commits July 24, 2017 23:09

request loading replaced with require

843d3a0

npn version of polyglot used

43cde1e

forgotten imports

a47dab3

erquhart mentioned this pull request Oct 23, 2017

Support for multilingual content authoring #716

Closed

tech4him1 mentioned this pull request Nov 9, 2017

Translation #789

Closed

polyglot abstracted to a module

77bb466

vencax mentioned this pull request Dec 30, 2017

i18n round 2 #974

Closed

vencax closed this Dec 30, 2017

j-rewerts mentioned this pull request Jun 5, 2019

Admin dashboard only supports english canada-ca/ore-ero#271

Closed


		const errors = []

		fs.readdirSync(i18nFolder).forEach(file => {

I18n for static strings in UI #403

I18n for static strings in UI #403

Conversation

vencax commented May 4, 2017

erquhart commented May 4, 2017

vencax commented May 9, 2017

vencax commented May 18, 2017

erquhart commented May 18, 2017

vencax commented May 19, 2017

Benaiah 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

Benaiah Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah Jun 23, 2017 • 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

vencax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benaiah commented Jul 21, 2017

erquhart commented Jul 22, 2017 • edited Loading

vencax commented Jul 22, 2017

erquhart commented Jul 24, 2017 • edited Loading

vencax commented Jul 24, 2017

vencax commented Jul 24, 2017

erquhart commented Jul 24, 2017 • edited Loading

erquhart commented Jul 24, 2017

vencax commented Jul 24, 2017

erquhart commented Sep 2, 2017

verythorough commented Dec 30, 2017

verythorough commented Dec 30, 2017

vencax commented Dec 30, 2017

Benaiah Jun 23, 2017 •

edited

Loading

Benaiah Jun 23, 2017 •

edited

Loading

erquhart commented Jul 22, 2017 •

edited

Loading

erquhart commented Jul 24, 2017 •

edited

Loading

erquhart commented Jul 24, 2017 •

edited

Loading