Localization #2195

alexcjohnson · 2017-12-07T05:20:43Z

Creates a framework for localization:
- Dictionaries can either be registered (in the bundle or afterward) or provided to a plot as config
- Locale to use must be provided as plot config
Wraps all displayed string literals in gd-specific localization function (did I miss any?)
Collects all translation key strings (on build or test) and barfs if any non-literals are translated
Does not localize date tick labels. This will be a separate PR, using the same config.locale but pulling world-calendars locale data in, so no additional translations will be needed there.

alexcjohnson · 2017-12-07T05:26:44Z

src/components/modebar/buttons.js

@@ -47,31 +49,31 @@ var modeBarButtons = module.exports = {};

 modeBarButtons.toImage = {
    name: 'toImage',
-    title: 'Download plot as a png',
+    title: function(gd) { return _(gd, 'Download plot as a png'); },


It's kind of annoying to have to turn all these button titles into functions... just to follow the rule that you can only translate a literal string. In principle we could do this translation in ModeBar.createButton by adding some complexity to the find_locale_strings task to allow one non-literal translation and look for keys inside modebarButtons.

I think that way you have here is cleaner than more special logic to find_locale_strings 👌

alexcjohnson · 2017-12-07T05:29:39Z

src/plots/cartesian/dragbox.js

@@ -296,7 +296,7 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) {
        dragTail(zoomMode);

        if(SHOWZOOMOUTTIP && gd.data && gd._context.showTips) {
-            Lib.notifier('Double-click to<br>zoom back out', 'long');
+            Lib.notifier(Lib._(gd, 'Double-click to<br>zoom back out'), 'long');


Seems funny to include   in the translation - should we 🔪 that and maybe just set notifier max width in css? (it's a <div> so we can do that 😄 )

should we 🔪 that and maybe just set notifier max width in css?

That would be lovely 👌

should we 🔪 that and maybe just set notifier max width in css?

Actually we already have it, and as currently set we just break at the next word:

I'm inclined to take the   out here and just leave it to break where it will. We can of course still use   in cases where it conveys some meaning, but this is not such a case.

  removed in 5d6f267

alexcjohnson · 2017-12-07T05:32:14Z

src/traces/box/calc.js

+                q1: _(gd, 'q1'),
+                q3: _(gd, 'q3'),
+                max: _(gd, 'max'),
+                mean: trace.boxmean === 'sd' ? _(gd, 'mean ± σ') : _(gd, 'mean'),


also not sure about making mean and mean ± σ separate translations... most languages will probably leave ± σ intact, but some might want to change it (Greek?)

alexcjohnson · 2017-12-07T05:33:11Z

src/traces/heatmap/calc.js

@@ -82,21 +83,21 @@ module.exports = function calc(gd, trace) {

    function noZsmooth(msg) {
        zsmooth = trace._input.zsmooth = trace.zsmooth = false;
-        Lib.notifier('cannot fast-zsmooth: ' + msg);
+        Lib.notifier(_(gd, 'cannot use zsmooth: "fast"') + '<br>' + msg);


Do we really want this (and the 3 msgs below) to be notified? Can I just turn it into a console warning and leave it in English?

Can I just turn it into a console warning and leave it in English?

I vote 👍

I think Lib.notifier should only be used for things that might affect the end user.

Lib.{log,warn,error} should suffice for messages that only help developers debug .

moved these to Lib.warn and turned log level 1 on by default in 0f8520e

see also #420 (comment)

alexcjohnson · 2017-12-07T05:33:55Z

src/traces/ohlc/transform.js

+    var openName = _(gd, 'Open') + ': ';
+    var highName = _(gd, 'High') + ': ';
+    var lowName = _(gd, 'Low') + ': ';
+    var closeName = _(gd, 'Close') + ': ';


Should : be part of the translation? (Applies a bunch of other places as well)

I suppose in principle one could make that argument about other punctuation as well (,, (, )...) but that seems like a can of worms we don't really want to open...

Should : be part of the translation?

I think so. For example, in french it is customary to add a nbsp before :, ;, ! and ? e.g. Open: 10 would be Ouverture : 10.

moved colons into the translation keys in bf2c6db

for old-style placeholder that contained the axis number caught by test image 19

etpinard · 2017-12-07T14:14:10Z

cc #856

etpinard

Looking great. I'm impressed with how little new code has been added 👌

I made a few comments. The most blocking one is in scattergeo.

etpinard · 2017-12-07T14:46:03Z

src/components/modebar/buttons.js

@@ -47,31 +49,31 @@ var modeBarButtons = module.exports = {};

 modeBarButtons.toImage = {
    name: 'toImage',
-    title: 'Download plot as a png',
+    title: function(gd) { return _(gd, 'Download plot as a png'); },


I think that way you have here is cleaner than more special logic to find_locale_strings 👌

etpinard · 2017-12-07T14:52:42Z

src/plot_api/plot_config.js

+
+    // Which localization should we use?
+    // Should be a string like 'en' or 'en-US'.
+    locale: 'en',


Hmm. I guess Click to enter Colorscale title would be Click to enter Colourscale title in non-US english. So should our default locale be en-US?

So should our default locale be en-US?

Ha sure - might be important for dates too. Then I can make and pre-register en and en-US dictionaries to override these appropriately.

default locale to 'en-US', with pre-registered dictionaries for this and 'en', in 04ed6f0.
This also conveniently documents the locale module format.

etpinard · 2017-12-07T14:56:56Z

dist/translation-keys.txt

@@ -0,0 +1,60 @@
+Autoscale


Quick question: are we planning on having a set on official dictionaries or should non en-US dictionaries be entirely handled by users?

are we planning on having a set on official dictionaries

They're not going to be very big, and seems really useful so I'd vote yes - with the caveat that they would probably mostly have to be contributed by users. We might have to make use of google translate during review, as a sanity check...

Also it occurs to me now, some of these keys might not be obvious out of context - perhaps I should comment each with the file & line it came from? Something like:

kde src/traces/violin/calc.js:73

and then we could comment there with more information // kernel density estimate (maybe I could even put that comment into translation-keys.txt?)

etpinard · 2017-12-07T14:57:44Z

src/plots/cartesian/dragbox.js

@@ -296,7 +296,7 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) {
        dragTail(zoomMode);

        if(SHOWZOOMOUTTIP && gd.data && gd._context.showTips) {
-            Lib.notifier('Double-click to<br>zoom back out', 'long');
+            Lib.notifier(Lib._(gd, 'Double-click to<br>zoom back out'), 'long');


should we 🔪 that and maybe just set notifier max width in css?

That would be lovely 👌

etpinard · 2017-12-07T15:00:57Z

src/plots/cartesian/set_convert.js

@@ -381,9 +381,6 @@ module.exports = function setConvert(ax, fullLayout) {
        }

        if(!isFinite(ax._m) || !isFinite(ax._b)) {
-            Lib.notifier(


Let me guess: you removed this so that we don't have to pass gd to setConvert ?

Regardless, I'm ok with making this just an (English-only) error 😉

etpinard · 2017-12-07T15:09:23Z

src/traces/heatmap/calc.js

@@ -82,21 +83,21 @@ module.exports = function calc(gd, trace) {

    function noZsmooth(msg) {
        zsmooth = trace._input.zsmooth = trace.zsmooth = false;
-        Lib.notifier('cannot fast-zsmooth: ' + msg);
+        Lib.notifier(_(gd, 'cannot use zsmooth: "fast"') + '<br>' + msg);


I think Lib.notifier should only be used for things that might affect the end user.

Lib.{log,warn,error} should suffice for messages that only help developers debug .

etpinard · 2017-12-07T15:19:16Z

src/traces/scattergeo/calc.js

+        calcTrace[0].t = {
+            labels: {
+                lat: _(gd, 'lat') + ': ',
+                lon: _(gd, 'lon') + ': '


This is an interesting case here. One could argue that labels lat and lon could be configured using a layout attribute e.g.

geo: { lonaxis: { hoverprefix: 'longitude' }, lataxis: { hoverprefix: 'latitude' } } }

So I ask: where do we draw the line between configurable hover axis labels and dictionaries?

cc #265

Seems to me if we were to allow configuring labels like this, it would work like the dfltTitle items - the translation result would be used as the default in supplyDefaults, but any other value provided with the figure gets used verbatim. We can't expect to translate user-provided strings, but for the most part it doesn't matter, usually translation is not about viewing other people's plots in your own language, but about making your own plot work and function the way you want it to.

Seems to me if we were to allow configuring labels like this, it would work like the dfltTitle items

Good point. There's no downside to allowing lat:/lon: to be translated via dictionaries even if we add {lat,lon}axis.hoverprefix at some point.

etpinard · 2017-12-07T15:21:55Z

tasks/find_locale_strings.js

+var localizeRE = /(^|[\.])(_|localize)$/;
+
+// main
+findLocaleStrings();


Lovely script 👌

etpinard · 2017-12-07T15:22:18Z

test/jasmine/assets/supply_defaults.js

+/**
+ * supplyDefaults that fills in necessary _context
+ */
+module.exports = function supplyDefaults(gd) {


etpinard · 2017-12-07T15:26:15Z

test/jasmine/tests/localize_test.js

+    });
+
+    it('does not generate an automatic base language dictionary in context', function(done) {
+        plot('fr', {'fr-QC': {fries: 'poutine'}})


Test case of the year 🏆

gives an example of what these locales should look like as modules

just let it break where it's going to break

alexcjohnson · 2017-12-08T01:39:01Z

dist/translation-keys.txt

+q3:                                                   // traces/box/calc.js:131
+trace                                                 // plots/plots.js:439
+upper fence:                                          // traces/box/calc.js:135


@etpinard @bpostlethwaite thoughts on this format, with the (first) source line for each key included for a translator to use for more context?

Also having all these strings together raises some questions about whether they're all actually what we want, or if any could be made more consistent:

Is the verb "Double-click" or "Double click"?

Most of the hover label names (the items ending in ':') are lowercase, but some (OHLC, sankey) are capitalized - is this what we want?

In making this PR I standardized the "Click to enter <item> title" ones to have <item> capitalized (previously one or two of them were lowercase) - is that correct?

Most of the Lib.notifier items have no punctuation but some of them do. Should we standardize?

thoughts on this format,

I like this format a lot. Nicely done 👌

Is the verb "Double-click" or "Double click"?

I have no preference.

Most of the hover label names (the items ending in ':') are lowercase, but some (OHLC, sankey) are capitalized - is this what we want?

I'd vote for standardizing to lower case.

I standardized the "Click to enter title" [...] is that correct?

fine by me.

Most of the Lib.notifier items have no punctuation but some of them do. Should we standardize?

No preference.

I made all the hover labels lower case, standardized on "Double-click" (when used as a verb, which is the only way we use it in plotly.js text), and tweaked a few other messages -> 97821e9

so geo_choropleth-text doesn't barf on its unrecognized locations

alexcjohnson · 2017-12-08T03:15:52Z

test/image/index.html

@@ -4,6 +4,9 @@
 <!-- this index file gets copied in to the image server docker -->
 <script type="text/javascript" src="../plotly.js/dist/extras/mathjax/MathJax.js?config=TeX-AMS-MML_SVG"></script>
 <script type="text/javascript" src="../plotly.js/build/plotly.js" charset="utf-8"></script>
+<script type="text/javascript">
+    Plotly.setPlotConfig({logging: 0});
+</script>


@etpinard I added this so geo_choropleth-text would render with no console logs (which trip the image server to throw an error). That way the image server keeps the same definition of "error" as it had before. I'm kind of inclined though to either:

Downgrade the Location with id *** does not have a matching topojson feature at this resolution. message from a Lib.warn to a Lib.log, or

Remove the offending ids from that mock. It's a lot though: ARB, BHR, CEB, CSS, EAP, EAS, ECA, ECS, EMU, EUU, FCS, HIC, HKG, HPC, LAC, LCN, LDC, LIC, LMC, LMY, MEA, MIC, MLT, MNA, NAC, NOC, OEC, OED, OSS, SAS, SGP, SSA, SSF, SST, UMC, WLD

Thoughts?

How about both?

Downgrade the Location with id *** does not have a matching topojson feature at this resolution. message from a Lib.warn to a Lib.log,

Yes, that should certainly be Lib.log 👌

Remove the offending ids from that mock. It's a lot though: ARB, BHR, CEB, CSS, EAP, EAS, ECA, ECS, EMU, EUU, FCS, HIC, HKG, HPC, LAC, LCN, LDC, LIC, LMC, LMY, MEA, MIC, MLT, MNA, NAC, NOC, OEC, OED, OSS, SAS, SGP, SSA, SSF, SST, UMC, WLD

I'd vote for removing the offending ids from the mock, but adding a jasmine test spying on Lib.log (if not done already). This part here could be done in a separate PR.

alright, did both.

missing location downgraded to Lib.log 4b8f980 (we have tests of these messages, modified in that commit)

removed offending ids anyway 8b9e7d8

etpinard · 2017-12-08T14:31:05Z

lib/locales.js

+
+'use strict';
+
+module.exports = [


Interesting. I think registering only one local (en-US) in the dist bundles should suffice, but then again as only the spelling of colo(u)rscale differs between the two, including both en and en-US is fine by me 👌

etpinard · 2017-12-08T14:41:59Z

lib/index-basic.js

@@ -15,4 +15,7 @@ Plotly.register([
    require('./pie')
 ]);

+// locales
+Plotly.register(require('./locales.js'));


Maybe it would better to put these Plotly.register calls in src/core.js so that we don't have to duplicate them?

That would also mean moving locale modules to a new src/locales/ directory, as we wouldn't want to require lib/ files from src/ right?

…es for dist

alexcjohnson · 2017-12-08T18:28:38Z

tasks/bundle.js

+        var outPath = path.join(constants.pathToDist, outName);
+        wrapLocale(file, outPath);
+    });
+});


@etpinard what do you think about this solution?

Turned the locales into json files

Explicitly loaded just 'en' and 'en-US' into the built bundles (ie I got rid of locales.js so that if/when we add more locales they will not automatically go into all the bundles) but we'll still put new locales into lib/ for use if you make your own bundle.

Built separate minified js files for each locale, so if you load via a script tag (from CDN or script) you can first load plotly.js and then in the next script tag load the locale you want.

Turned the locales into json files

Requiring JSON files isn't ideal as described in #2195 (comment)

I'd vote for converting those lib/locale-??? files into plain js files.

so that if/when we add more locales they will not automatically go into all the bundles) but we'll still put new locales into lib/ for use if you make your own bundle.

great 👌

Built separate minified js files for each locale, so if you load via a script tag (from CDN or script) you can first load plotly.js and then in the next script tag load the locale you want.

Lovely ❤️ and by the looks of it out push-to-cdn script should just work^tm.

etpinard · 2017-12-08T18:56:25Z

lib/index-basic.js

-Plotly.register(require('./locales.js'));
+Plotly.register([
+    require('./locale-en.json'),
+    require('./locale-en-us.json')


Ha. Webpack users won't like that. Webpack (v1 at least) needs a special loader to bundle JSON files unlike browserify. See #183

ok, back to js 1275661

etpinard · 2017-12-08T19:11:06Z

tasks/util/wrap_locale.js

+        .pipe(fs.createWriteStream(pathToOutput))
+        .on('finish', function() {
+            logger(pathToOutput);
+        });


Smooth work with add-stream and info-stream. It would be nice to rewrite the addHeadersInDistFiles step in tasks/header.js using streams for 🐎

because it doesn't really seem to be an error, things work fine even when this is triggered at least in updatemenus.json

alexcjohnson · 2017-12-08T21:14:47Z

src/plots/command.js

@@ -114,7 +114,7 @@ exports.manageCommandObserver = function(gd, container, commandList, onchange) {
    } else {
        // TODO: It'd be really neat to actually give a *reason* for this, but at least a warning
        // is a start
-        Lib.warn('Unable to automatically bind plot updates to API command');
+        Lib.log('Unable to automatically bind plot updates to API command');


updatemenus.json emits this twice, and yet everything in its menus seems to work as expected... not sure what this is really about but since it doesn't (necessarily) mean anything is broken it seems more appropriate as a log than a warn.

it seems more appropriate as a log than a warn.

I agree 👌

jackparmer · 2017-12-10T02:06:28Z

woot! A mere 95 files changed! 😄 📚

etpinard · 2017-12-11T15:03:24Z

💃 nicely done

alexcjohnson added 4 commits December 7, 2017 00:07

localization routine and tests

7e075f5

remove zsmooth that doesn't apply to contourcarpet

9a9de77

wrap all displayed strings in localize

d859de5

task to collect translation keys and verify syntax

7d057e5

alexcjohnson added status: reviewable type: feature labels Dec 7, 2017

alexcjohnson added this to the v1.32.0 milestone Dec 7, 2017

alexcjohnson commented Dec 7, 2017

View reviewed changes

looser placeholder check in titles

8a5eeec

for old-style placeholder that contained the axis number caught by test image 19

lint

03dcb5d

etpinard reviewed Dec 7, 2017

View reviewed changes

alexcjohnson added 5 commits December 7, 2017 11:51

move colons into the translations

bf2c6db

set default locale to en-US and build this into the library

04ed6f0

gives an example of what these locales should look like as modules

turn log level 1 on by default and move zsmooth messages to logs

0f8520e

take out of notifier messages

5d6f267

just let it break where it's going to break

include file & line number with translation keys

97c0898

alexcjohnson commented Dec 8, 2017

View reviewed changes

disable logging in image server

daf0884

so geo_choropleth-text doesn't barf on its unrecognized locations

alexcjohnson commented Dec 8, 2017

View reviewed changes

etpinard reviewed Dec 8, 2017

View reviewed changes

alexcjohnson added 2 commits December 8, 2017 12:38

convert locales to json, only en(US)? in bundles, and build all local…

4b48038

…es for dist

move geo missing id warn -> log

4b8f980

alexcjohnson commented Dec 8, 2017

View reviewed changes

etpinard reviewed Dec 8, 2017

View reviewed changes

alexcjohnson added 2 commits December 8, 2017 15:43

convert locales back into js instead of json

1275661

change "unable to bind..." warning to a log

fc03cce

because it doesn't really seem to be an error, things work fine even when this is triggered at least in updatemenus.json

alexcjohnson commented Dec 8, 2017

View reviewed changes

alexcjohnson added 6 commits December 8, 2017 16:32

fix command test for warn -> log

cb2f00c

tweak a few of the string literals for consistency

97821e9

fix sankey test and clean log messages from other tests

e80ec91

remove invalid data from geo_choropleth-text.json

8b9e7d8

update finance test for lowercase labels

6029f94

update select_test for choropleth mock change

9708b56

alexcjohnson merged commit 599dff9 into master Dec 11, 2017

alexcjohnson deleted the i18n branch December 11, 2017 15:40

alexcjohnson mentioned this pull request Dec 12, 2017

Long words in notifier messages can go off the edge #2201

Closed

alexcjohnson added a commit that referenced this pull request Dec 15, 2017

use d3.locale formats just like the localization dicts of #2195

eacc99b

This was referenced Dec 15, 2017

Date and number localization #2207

Merged

Provide proper i18n support #856

Closed

alexcjohnson mentioned this pull request Jan 16, 2018

candlestick graph hoverinfo shows strange labels for open/close/low/high #2253

Closed

Localization #2195

Localization #2195

Conversation

alexcjohnson commented Dec 7, 2017

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

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

etpinard commented Dec 7, 2017

etpinard 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

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

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

jackparmer commented Dec 10, 2017

etpinard commented Dec 11, 2017