add support for number format internationalization #14019

fbaligand · 2017-09-16T08:43:12Z

This PR adds a new advanced setting named "format:language".
This setting allows to choose a language to format numbers, among all numeraljs supported languages.
Number format is about decimal delimiter, thousand delimiter and currency symbol.

This feature is particularly useful to format every language specific number formats (not only english).

Precision : given how numeraljs works, it could only be a global option, this can't be customized for each field. Up to me, not a problem since it would be weird to display on a same dashboard different numeral fields with different language formats.

Fixes #9228.
Partially fixes #8983.

timroes · 2017-09-18T08:51:45Z

src/core_plugins/kibana/ui_setting_defaults.js

+      value: 'en',
+      type: 'select',
+      options: ['be-nl', 'chs', 'cs', 'da-dk', 'de-ch', 'de', 'en', 'en-gb', 'es-ES', 'es', 'et', 'fi', 'fr-CA', 'fr-ch', 'fr', 'hu', 'it', 'ja', 'nl-nl', 'pl', 'pt-br', 'pt-pt', 'ru-UA', 'ru', 'sk', 'th', 'tr', 'uk-UA'],
+      description: '<a href="http://numeraljs.com/" target="_blank">numeral language</a>'


Could you please add rel="noopener" to this link.

timroes · 2017-09-18T08:53:24Z

src/core_plugins/kibana/common/field_formats/types/_numeral.js

@@ -26,6 +26,15 @@ export class Numeral extends FieldFormat {
      }

      getParamDefaults() {
+        var language = this.getConfig('format:language');


var -> const

timroes · 2017-09-18T08:53:55Z

src/core_plugins/kibana/common/field_formats/types/_numeral.js

@@ -26,6 +26,15 @@ export class Numeral extends FieldFormat {
      }

      getParamDefaults() {
+        var language = this.getConfig('format:language');
+        if (language != numeral.language()) {


!= → !==

timroes · 2017-09-18T08:54:12Z

src/core_plugins/kibana/common/field_formats/types/_numeral.js

+        var language = this.getConfig('format:language');
+        if (language != numeral.language()) {
+          if (language != 'en') {
+            var languageData = require('@elastic/numeral/languages/' + language);


Please use a template string here.

timroes · 2017-09-18T08:54:26Z

src/core_plugins/kibana/common/field_formats/types/_numeral.js

@@ -26,6 +26,15 @@ export class Numeral extends FieldFormat {
      }

      getParamDefaults() {
+        var language = this.getConfig('format:language');
+        if (language != numeral.language()) {
+          if (language != 'en') {


!= → !==

fbaligand · 2017-09-18T16:39:12Z

@timroes
Done !

timroes

LGTM

kobelb · 2017-09-19T11:17:15Z

src/core_plugins/kibana/ui_setting_defaults.js

@@ -226,6 +226,12 @@ export function getUiSettingDefaults() {
      value: '($0,0.[00])',
      description: 'Default <a href="http://numeraljs.com/" target="_blank">numeral format</a> for the "currency" format'
    },
+    'format:language': {


Should we consider moving this to format:numeral:language as this is only where it's used currently?

👍 to making this setting more specific. I'm also slightly concerned about how this is a global setting. Perhaps format:number:defaultLanguage will keep it in line with format:number:defaultPattern and leaves room for a per-field language setting should we want to add one down the road.

Hi,

format:number:defaultLanguage looks a good idea.
That said, few points about that :

first, this setting will apply on every type format concerned by numeraljs, so : number, bytes, percent, and currency. Not only number.

then, currently (and since a long time), numeraljs allows only to define a global static language, not one for each formatter.

would it make sense to have a dashboard with different numeric fields formatted with different languages ?

in the future, an interesting evolution could be to detect browser language.

=> Given these points, what setting name do you choose ? :)

numeraljs allows only to define a global static language

ugh, bummer. Is it unrealistic to call numeral.language() before every conversion?

would it make sense to have a dashboard with different numeric fields formatted with different languages?

The numeral language defines the currency symbol right? If so I think it makes sense to have a unique language per field if two fields represent different currencies.

Looks like numeral.language() is just setting an internal value inside the library, so if we called it before calling numeralInst.set().format(this.param('pattern')) we could use a different language for each field.

If we do move forward with applying this language to all instances of numeral, it'd be more consistent to do so in the manner described in this comment

@spalger I agree with you, it's fine to have defaultLanguage here to be able to add per-field value later.
And yes, numeral language defines the currency symbol.
That said, in original numeraljs project (https://github.com/adamwdraper/Numeral-js), the last version use the "locale" term instead of "language" term.
So question is : do you prefer format:number:defaultLanguage or format:number:defaultLocale

I'm not too concerned about updates in numeral; I don't think we'll ever upgrade because of it's breaking changes and how that would impact everyone who has saved formats already. It's more likely that we will do something like we have done with kuery and offer a choice between legacy numeral and some custom solution we could build in the future.

That said, I think that locale is more accurate description of the choice, so I would go with format:number:defaultLocale

OK, for this new name format:number:defaultLocale, I will commit it.

@spalger
new name format:number:defaultLocale is commited

kobelb · 2017-09-19T11:30:05Z

src/core_plugins/kibana/common/field_formats/types/_numeral.js

+        const language = this.getConfig('format:language');
+        if (language !== numeral.language()) {
+          if (language !== 'en') {
+            var languageData = require(`@elastic/numeral/languages/${language}`);


Replace var usage with const per our style-guide https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#use-const-for-variables

@kobelb
Sorry for this mistalke. I just pushed the fix.

kobelb · 2017-09-19T12:22:32Z

src/core_plugins/kibana/common/field_formats/types/_numeral.js

@@ -26,6 +26,15 @@ export class Numeral extends FieldFormat {
      }

      getParamDefaults() {
+        const language = this.getConfig('format:language');


We really shouldn't be setting the numeral language here, this method is only meant to return the param defaults. This situation appears to be very similar to setting the timezone for moment which is being done in the KibanaRootController here but then we're also having to do so in other app controllers, example here.

Perhaps @spalger can comment on a better place to put logic like this without these deficiencies.

Additionally, I have hesitations to setting the language for the global instance of numeral as this could have unintended side-effects outside of modifying the behavior of field formatters specifically.

I agree it's probably not the best place to set this code.
If you find a better place, I would be glad to hear it.
I set it there because this method is called at app loading beginning and is rarely called then.

Concerning your hesitations, currently, numeraljs allows only to set this setting globally, not locally for each formatter. Then, would it make sense to have several language formats on the same kibana app ?

This thread is kind of merging with #14019 (comment)

@spalger do you see a better place (method) to put the javascript code which changes numeral language ?

I'd just put it in the _convert function, then adding per-field params is just about exposing the UI to do so.

If I set it there, it would require static import of all languages.
Because, dynamic require call shouldn't be called at convert time, as it is an asynchronous load.

kobelb · 2017-09-19T12:23:27Z

src/core_plugins/kibana/ui_setting_defaults.js

+    'format:language': {
+      value: 'en',
+      type: 'select',
+      options: ['be-nl', 'chs', 'cs', 'da-dk', 'de-ch', 'de', 'en', 'en-gb', 'es-ES', 'es', 'et', 'fi', 'fr-CA', 'fr-ch', 'fr', 'hu', 'it', 'ja', 'nl-nl', 'pl', 'pt-br', 'pt-pt', 'ru-UA', 'ru', 'sk', 'th', 'tr', 'uk-UA'],


It's a shame that we're hard-coding these, but I'm not seeing a way to access all possible languages...

@kobelb
I think exactly like you :)
I'm sad to hard-code these values, but I didn't find another way to do that.
That said, this array is likely to change very rarely (maybe never)...

fbaligand · 2017-09-20T17:39:34Z

src/core_plugins/kibana/common/field_formats/types/_numeral.js

+        const language = this.getConfig('format:language');
+        if (language !== numeral.language()) {
+          if (language !== 'en') {
+            const languageData = require(`@elastic/numeral/languages/${language}`);


@spalger @kobelb
Do you see a better way to load language data ?
Ideally, I would like to load statically all languages data to store them in numeral language map.
I try import from @elastic/numeral/languages'; but it doesn't work.

@fbaligand feel free to add the ability to do so at https://github.com/elastic/numeral-js

@spalger
One option for that is to use v3.0.0 that is already released. This version is compatible with static import. We can change the package.json file to depend on this version.
Are you OK to depend on this version ? Or do you consider this major version implies so much breaking changes ?

@spalger
I just commited migration to @elastic/numeral v3.0.0
I had no particular problem, it works fine !
And now I can import successfully all languages using a static way.
And so, I moved the code to _convert method as you wish.

@spalger
What do you think about ?

Sorry, I should have deprecated @elastic/[email protected] months ago. Unfortunately the way that numeral transforms format strings in v3 is not backwards compatible with v2. This means that in order to upgrade to numeral v3 we would either have to track down all the differences between the two versions, or users to recreate/double-check all of their numeric field formatters. We're currently not planning to do either, so this isn't going to work. I've updated elastic/numeral-js to reflect the current state of the package and changed https://github.com/elastic/numeral-js/blob/kibana-fork/languages.js

The file now exports the languages as an array. each item has an id, name, and lang property.

id: the filename of the lang

name: the english description of the language, extracted from each language file

lang: the value that should be passed to numeral.language(id, lang)

The file doesn't automatically register the languages, maybe we want it to do that, but it should be a suitable replacement for the current languages import, right? To test it out install @elastic/numeral from github:

npm install https://github.com/elastic/numeral-js#kibana-fork

@spalger
OK.
Thanks for all your work to transform "languages.js" in a way it can be imported easily !
So I'm ready to cancel my upgrade to 3.0.0 and write the code to register all languages in _numeral.js using your new languages.js version.
Hope to test it tonight.

spalger · 2017-09-27T17:48:54Z

The conversation is flattened in my view of the PR, but I responded here

fbaligand · 2017-09-27T18:14:25Z

@spalger
As you, I answered to the "old" conversation here.
Next time, if you prefer that I write a new comment, tell me.

fbaligand · 2017-09-28T21:29:20Z

@spalger
OK, I just cancelled my upgrade to 3.0.0 and wrote the code to register at startup, all languages in _numeral.js using your new languages.js version.
=> what do you think about ?

spalger · 2017-09-29T00:13:52Z

src/core_plugins/kibana/ui_setting_defaults.js

+    'format:number:defaultLocale': {
+      value: 'en',
+      type: 'select',
+      options: ['be-nl', 'chs', 'cs', 'da-dk', 'de-ch', 'de', 'en', 'en-gb', 'es-ES', 'es', 'et', 'fi', 'fr-CA', 'fr-ch', 'fr', 'hu', 'it', 'ja', 'nl-nl', 'pl', 'pt-br', 'pt-pt', 'ru-UA', 'ru', 'sk', 'th', 'tr', 'uk-UA'],


We should be able to replace this array with the ids from @elastic/numeral/languages right?

Yes, it would be great to dynamically inject all language ids from "languages.js" array.
That said, this configuration file seems to be completely static.
=> do you know how to dynamically object values here ?

You can require files at the top just like everything else, it's not actually static, just doesn't have any dependencies yet.

You're right, sorry for this remark. Even more, I just saw that there is already one dependency.
I will fix this file to dynamically inject array from "languages.js".

spalger · 2017-09-29T00:14:09Z

Jenkins, test this

fbaligand · 2017-09-29T06:16:52Z

@spalger
So that Jenkins build works, it should require a new release of "numeral" package, right ?

spalger · 2017-09-29T06:18:36Z

Looks like jenkins build failed for linting reasons (run npm run lint locally) but you're right, I need to publish that.

spalger · 2017-09-29T06:20:44Z

Just published @elastic/numeral 2.3.0

fbaligand · 2017-09-29T07:37:37Z

Thanks for the release !
I will import it, and fix the lint issues detected by Jenkins

fbaligand · 2017-09-29T10:24:02Z

@spalger
I just fixed jenkins lint issues, changed dependency to '@elastic/numeral' version 2.3.0, and injected dynamically language ids into format:number:defaultLocale setting options.

I guess I can't launch Jenkins tests for myself. Could you launch Jenkins tests ?

spalger · 2017-09-29T22:18:58Z

Jenkins, test this

fbaligand · 2017-09-30T08:42:37Z

@spalger
Given Jenkins build logs, there are not lint errors anymore.

There are errors during tests. The main problem seems to be :

22:29:22 Chrome 59.0.3071 (Linux 0.0.0) Range Agg formating formats bucket keys properly FAILED
22:29:22 	TypeError: this.getConfig is not a function
22:29:22 	    at Class._convert (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=2:420651:33)
22:29:22 	    at recurse (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=2:191577:25)
22:29:22 	    at FieldFormatFromConverter._convert (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=2:308585:46)
22:29:22 	    at recurse (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=2:191577:25)
22:29:22 	    at format (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=2:473843:37)
22:29:22 	    at Context.<anonymous> (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=2:473845:30)

I don't arrive to localize this test in Kibana sources, given the test label.
But it seems this error is because FieldFormatFromConverter calls directly FieldFormat, so it doesn't call _numeral Class constructor with getConfig parameter. That explains error above.

Honestly, I don't know how to fix this test. Any idea ?

fbaligand · 2017-09-30T16:09:13Z

@spalger
A way to work anyway would be to put this code line :
const defaultLocale = this.getConfig && this.getConfig('format:number:defaultLocale') || 'en';

=> What do you think about ?

fbaligand · 2017-10-06T12:37:44Z

@spalger
I just published const defaultLocale = this.getConfig && this.getConfig('format:number:defaultLocale') || 'en'; to fix jenkins tests.
=> could you launch jenkins tests ?

kobelb · 2017-10-10T11:52:24Z

jenkins, test this

fbaligand · 2017-10-10T21:31:23Z

@spalger @kobelb @nreese
Yes, jenkins build is OK !

=> Is this OK to merge the PR ?

nreese

LGTM

kobelb · 2017-10-11T13:03:42Z

I believe I brought this up earlier, and it might've already been addressed... are we not concerned that we're modifying the global numeral language from within the numeral field_formatter? It's conceivable that another Kibana application would be relying upon Numeral to do it's own formatting, and this would affect it's behavior as well.

This could be incredibly confusing to the user in the following scenario. If there were two applications Kibana: App-1 relies upon numeraljs and NOT our field formatters; App-2 relies upon our field formatters and in turn numeraljs. When the user first opens Kibana they go directly to App-1, bypassing the field-formatter code that sets the default language. They then navigate to App-2 which is setting the global numeral language. Later if they go back to App-1 they see the number formats change.

spalger · 2017-11-21T23:00:40Z

Ah, sorry about that @fbaligand go ahead and keep your version.

Can you try running the tests locally and see if they fail?

In one terminal tab run:

npm run test:ui:server

Once the Kibana server has started up in the first terminal run this in a second tab:

node scripts/functional_test_runner --grep "management scripted fields"

That will run the tests in test/functional/apps/management/_scripted_fields.js and might give you an indication if this is caused be the number formatters changing (a possibility if you ask me)

fbaligand · 2017-11-21T23:19:08Z

@spalger
I just pushed my rebased code, which fixes the removed code "opts.afterConvert" from your merge.
Maybe it was the cause of the failed selenium test ?
=> Could you run jenkins tests to see if build is fixed by my last push ?

Now, I have to go to sleep ;)

spalger · 2017-11-21T23:49:02Z

Okay, that failure wasn't because of your code, for sure. One more jenkins run should do it

fbaligand · 2017-11-22T06:27:46Z

@kobelb @spalger
Yeah! Jenkins build is successful!

=> so now, what's the next step?

fbaligand · 2017-11-22T12:42:17Z

Thanks @kobelb for the approval !

fbaligand · 2017-11-22T12:42:43Z

@spalger @kobelb
So is it OK to merge the PR ?

kobelb · 2017-11-22T13:05:46Z

@fbaligand yup! I can merge this a little later today. Thanks for working through this with us and your patience.

fbaligand · 2017-11-22T13:14:16Z

Great !
It was a pleasure to work with all of you on this PR !

fbaligand · 2017-11-22T23:19:33Z

@spalger
Thanks for the merge !!
Very happy that this feature is now merged and soon part of Kibana 6.2 !

fbaligand · 2017-11-22T23:23:16Z

Is it too late to merge it in Kibana 6.1 ?

nreese · 2017-11-25T01:40:35Z

Is it too late to merge it in Kibana 6.1 ?

Yes

fbaligand · 2017-11-25T02:16:38Z

Ok. Thanks for the answer @nreese.

…5132)

spalger · 2017-11-30T22:42:45Z

6.x/6.2: 91777d2

leeway23 · 2018-04-12T15:41:10Z

Hi all,

would anyone know from which Kibana version a problem previously discussed in (and later merged with this discussion) :
#7543
is going to be fixed ?

Thanks & Cheers, J.

MarcelZOI · 2018-08-28T07:29:55Z

Hey, i currently looking how to change the number format of a column in visualize to german format. what is the current state? Is there a possibility to achive this?
Greetings

Marcel

(currently i use kibana 5.1.1 and i wanna know is this feature released and it is worth to upgrade?)

fbaligand · 2018-08-28T09:38:59Z

Hi @MarcelZOI

As you can see in this PR tags, this feature is available since Kibana 6.2.0.

Important point : this setting is a Kibana advanced setting, so it is global. You currently can't have a custom locale per field.

MarcelZOI · 2018-08-28T10:20:30Z

@fbaligand okay, thanks.

timroes suggested changes Sep 18, 2017

View reviewed changes

timroes added :Management release_note:enhancement labels Sep 18, 2017

timroes approved these changes Sep 19, 2017

View reviewed changes

timroes requested review from nreese, kobelb and spalger September 19, 2017 10:19

kobelb reviewed Sep 19, 2017

View reviewed changes

fbaligand commented Sep 20, 2017

View reviewed changes

nreese mentioned this pull request Sep 21, 2017

Field format - foreign currency #4392

Closed

fbaligand force-pushed the numeral-language branch from f6d8f5e to f6277e3 Compare September 28, 2017 21:26

spalger reviewed Sep 29, 2017

View reviewed changes

nreese approved these changes Oct 11, 2017

View reviewed changes

fbaligand force-pushed the numeral-language branch from 4442c3f to 5f3c8f5 Compare November 21, 2017 23:13

kobelb approved these changes Nov 22, 2017

View reviewed changes

spalger merged commit 0420cd7 into elastic:master Nov 22, 2017

spalger added v6.2.0 v7.0.0 labels Nov 22, 2017

spalger pushed a commit to spalger/kibana that referenced this pull request Nov 22, 2017

add support for number format internationalization (elastic#14019)

7fa8661

spalger mentioned this pull request Nov 22, 2017

[6.x] add support for number format internationalization (#14019) #15132

Merged

fbaligand deleted the numeral-language branch November 25, 2017 18:04

spalger added a commit that referenced this pull request Nov 30, 2017

[6.x] add support for number format internationalization (#14019) (#1…

91777d2

…5132)

fbaligand mentioned this pull request Jun 11, 2018

Add format:defaultLanguage in Advanced Settings #9228

Closed

bphenriques mentioned this pull request Aug 9, 2018

[Bug]: Vega number locale should be consistent with Kibana's format:number:Locale nyurik/kibana-vega-vis#79

Open

fbaligand mentioned this pull request May 27, 2020

Canvas does not use Kibana number formatting locale #28007

Closed

add support for number format internationalization #14019

add support for number format internationalization #14019

Conversation

fbaligand commented Sep 16, 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

fbaligand commented Sep 18, 2017

timroes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbaligand Sep 19, 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

fbaligand Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbaligand Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

fbaligand Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb Sep 19, 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

fbaligand Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbaligand Sep 19, 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

fbaligand Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

spalger commented Sep 27, 2017

fbaligand commented Sep 27, 2017

fbaligand commented Sep 28, 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

spalger commented Sep 29, 2017

fbaligand commented Sep 29, 2017

spalger commented Sep 29, 2017

spalger commented Sep 29, 2017 • edited Loading

fbaligand commented Sep 29, 2017

fbaligand commented Sep 29, 2017 • edited Loading

spalger commented Sep 29, 2017

fbaligand commented Sep 30, 2017

fbaligand commented Sep 30, 2017 • edited Loading

fbaligand commented Oct 6, 2017

kobelb commented Oct 10, 2017

fbaligand commented Oct 10, 2017

nreese left a comment

Choose a reason for hiding this comment

kobelb commented Oct 11, 2017

spalger commented Nov 21, 2017

fbaligand commented Nov 21, 2017

spalger commented Nov 21, 2017

fbaligand commented Nov 22, 2017

fbaligand commented Nov 22, 2017

fbaligand commented Nov 22, 2017

kobelb commented Nov 22, 2017

fbaligand commented Nov 22, 2017

fbaligand commented Nov 22, 2017

fbaligand commented Nov 22, 2017

nreese commented Nov 25, 2017

fbaligand commented Sep 16, 2017 •

edited

Loading

fbaligand Sep 19, 2017 •

edited

Loading

fbaligand Sep 20, 2017 •

edited

Loading

fbaligand Sep 24, 2017 •

edited

Loading

fbaligand Sep 26, 2017 •

edited

Loading

kobelb Sep 19, 2017 •

edited

Loading

fbaligand Sep 24, 2017 •

edited

Loading

fbaligand Sep 19, 2017 •

edited

Loading

fbaligand Sep 27, 2017 •

edited

Loading

spalger commented Sep 29, 2017 •

edited

Loading

fbaligand commented Sep 29, 2017 •

edited

Loading

fbaligand commented Sep 30, 2017 •

edited

Loading

MarcelZOI commented Aug 28, 2018 •

edited

Loading