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

Properly localize numbers #8445

Closed
martinpitt opened this issue Jan 18, 2018 · 2 comments
Closed

Properly localize numbers #8445

martinpitt opened this issue Jan 18, 2018 · 2 comments
Labels
good-first-issue Appropriate for new contributors i18n

Comments

@martinpitt
Copy link
Member

We currently don't properly localize numbers with thousands or decimal separators. E. g. when switching language to German, the network page shows:

numbers-net

and the storage page shows:

numbers-storage

Our localization API does not have a function for formatting numbers according to the current locale, but I figure we don't really need one: Our code could just use

>> n = 1234.56
>> n.toLocaleString(cockpit.language)
"1.234,56"

If we add an API to base, we need some fallback anyway as we might run against an older cockpit-bridge package (like on RHEL or with containers).

@martinpitt martinpitt added good-first-issue Appropriate for new contributors i18n labels Jan 18, 2018
@martinpitt martinpitt assigned martinpitt and unassigned martinpitt Apr 9, 2018
@allisonkarlitskaya
Copy link
Member

cockpit.format_number() in already contains this comment:

    /* TODO: Make the decimal separator translatable */

    /* We show 3 digits of precison but avoid scientific notation.
     * We also show integers without digits after the comma.
     */

Would implementing this change there be considered an API break in your opinion? If not, do we basically need to go all over the place, changing each separate location?

@martinpitt
Copy link
Member Author

martinpitt commented Apr 10, 2018

format_number() is explicitly for turning a Number into a humanly readable string, so respecting the locale there is IMHO not an API break. There's also precedent: In commit 0218d77 we recently changed the precision of that function.

IMHO fixing it centrally is better than wrapping all invocations of this with toLocaleString(). This also provides better consistency when mixing newer cockpit-* extension packages with an older cockpit-system (as happens in RHEL).

allisonkarlitskaya added a commit to allisonkarlitskaya/cockpit that referenced this issue Apr 11, 2018
Modify format_number() to use .toLocaleString() to format a
locale-correct decimal separator, while avoiding thousands separators.

MDN says that the 'options' parameter is not supported on at least some
versions of mobile browsers, so we might have a minor regression there,
but this is not a huge issue, and nicer than having more complicated
code.

Expand the test-format testcase to re-run all of the tests under locale
'de' to ensure that we are getting properly-formatted numbers in that
case (ie: comma as the decimal separator).

Closes cockpit-project#8960
Fixes cockpit-project#8445
martinpitt pushed a commit that referenced this issue Apr 11, 2018
Modify format_number() to use .toLocaleString() to format a
locale-correct decimal separator, while avoiding thousands separators.

MDN says that the 'options' parameter is not supported on at least some
versions of mobile browsers, so we might have a minor regression there,
but this is not a huge issue, and nicer than having more complicated
code.

Expand the test-format testcase to re-run all of the tests under locale
'de' to ensure that we are getting properly-formatted numbers in that
case (ie: comma as the decimal separator).

Fixes #8445
Closes #8960
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Appropriate for new contributors i18n
Projects
None yet
Development

No branches or pull requests

2 participants