-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Format numbers by their locale #5728
Conversation
For reference, we talked about this last in #1257 and I think there were two themes about why we left it as-is:
The work required to understand the domain well enough, to then write a native module to read the locale info from the OS-specific APIs, and finally get enough test coverage of what users might do wasn't quite worth it then. Maybe if we get more interest in proper date formatting (I'm not sure what Momentjs does here, but I suspect it's limited by browser support too) this could become a more practical investment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the harm in doing this. It may not work for all configurations, but its at worst status quo.
As @shiftkey said we decided against this approach in #1257 and I'm not sure what's changed since then that would make us reconsider? If we can get proper number formatting that takes into account the number formatting locale I'd be super happy to merge but unless something has changed that I'm missing here I don't think we have that information at our disposal at the moment.
Could you elaborate on this? For me it would render the example in the PR body as My locale settings What this PR proposes What it should be |
thanks @niik that helps a lot to clarify the problem. @iAmWillShepherd do you have thoughts? |
for what its worth, the following also works
|
@outofambit this hasn't been address in Electron or Chromium(?), so there's not much we can do at the moment. I'm going to close this out until a fix is implemented in our dependencies. |
Fixes #1245
Related issue: electron/electron#9247
I took a shot in the dark on this, but I understand that using the
toLocaleString
function may not be sufficient, so it would be great if we can discuss the reasons why it's not sufficient or better approaches to formatting large numbers.Before
After
cc @desktop/engineering