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

Localize all numbers in report renderer #3115

Closed
Garbee opened this issue Aug 24, 2017 · 6 comments
Closed

Localize all numbers in report renderer #3115

Garbee opened this issue Aug 24, 2017 · 6 comments
Labels

Comments

@Garbee
Copy link
Contributor

Garbee commented Aug 24, 2017

As a question on StackOverflow brings up, the number formatting within Lighthouse isn't locale specific. Currently undefined is passed as the locale which essentially translates to ''. As far as what happens from there I'm not too sure sifting through the ECMAScript specification.

Either way, one solution for rendering it out properly would be to pass in the navigator's current locale if defined. So something like:

 static formatNumber(number, decimalPlaces = 1) {
    const locale = typeof navigator === 'undefined' ? undefined : navigator.language;
    return number.toLocaleString(locale, {maximumFractionDigits: decimalPlaces});
  }
@patrickhulce
Copy link
Collaborator

This is a good find :) this seems more like an issue with the node locale not matching the Chrome locale to me though. For the CLI, some of the numbers are converted into strings in node and others are put into the report as numbers and only turned into strings by the report renderer in the browser. The browser ones appear to be fine but the node ones are erroneously english due to the lack of node i18n support.

@Garbee
Copy link
Contributor Author

Garbee commented Aug 24, 2017

Node doesn't have navigator which is why the guard is in place and setting that to undefined. I assume that would end up defaulting to en-US in the end.

If anyone has a method to get the right locale in node then we could easily upgrade the check later (possibly make a method for getting the locale to only write that logic in one place.) For now though, handling the in-browser rendering when you run a test via DevTools and the extension should be fine enough to start making things more user-friendly.

@patrickhulce
Copy link
Collaborator

My point is that the in-browser strings are already being handled properly in the report and the fix is not to pass navigator.language (which is taken from the OS in Chrome) but to instead not generate the strings under two different locales. We can just localize all in the browser.

From MDN

If the argument is undefined, this method returns localized digits specified by the OS

@patrickhulce patrickhulce changed the title User locale for number formatting Localize all numbers in report renderer Aug 24, 2017
@Garbee
Copy link
Contributor Author

Garbee commented Aug 24, 2017

Ah, missed that part reading over MDN's details. Thanks for pointing it out.

@paulirish
Copy link
Member

paulirish commented Sep 28, 2017

p2 fix is forcing 'en-us'.
p3 fix is not shipping displayValues and formatted values in the JSON. This is a major-version breaking change, probably;

@brendankenny
Copy link
Member

mostly done! Moving over to #5719 for the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants