feat: use Number.toLocaleString to support localization #216
Conversation
|
Thank you @pimlie! It looks like related unit tests are failing. Could you double check them please. |
|
Hi @jdalton, PhantomJS doesnt seem to support Also I simplified the travis config a bit (although opinions may vary), it work nicely (except for the phantomjs issues) but before I 'finalize' that config maybe you are opposed against using npm scripts like this and prefer the old config? |
|
Dropping phantom is fine 😄 |
|
Great thanks, even better 👍 Let me know if you have any other remarks! |
| return number[0].replace(/(?=(?:\d{3})+$)(?!\b)/g, ',') + | ||
| (number[1] ? '.' + number[1] : ''); | ||
| function formatNumber(number, locale) { | ||
| return number.toLocaleString(locale); | ||
| } |
There was a problem hiding this comment.
Could you add a check during the creation of formatNumber that Number.prototype.toLocaleString is a function and if it's not then make formatNumber fallback to the legacy formatter?
There was a problem hiding this comment.
Is like this ok or did you mean something else with during the creation of formatNumber?
You could switch to Puppeteer. With some additional test-refactoring effort, of course... |
Resolves: #210
This PR add locale support by using Number.toLocaleString for number formatting.
Btw, as we discussed in the corresponding issue I did some work on adapting a polyfill for Number.toLocaleString but unfortunately I couldnt get it to fully work correctly in the time I had set for this (the polyfill works actually but when used as a dependency it doesnt download the correct cldr-data due to an issue in the cldr-data-npm package). As most (current?) browsers fully support number localization already I think the full-icu solution will work just fine :)