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

Extension install count, use current locale for number formatting Fixes #29491 #29783

Merged
merged 2 commits into from
Jul 25, 2017

Conversation

cleidigh
Copy link
Contributor

See #29491 for discussion
sorry had to squash commits

@joaomoreno joaomoreno added this to the Backlog milestone Jun 29, 2017
@joaomoreno joaomoreno assigned joaomoreno and sandy081 and unassigned joaomoreno Jun 29, 2017
@cleidigh cleidigh changed the title extension install count formatting Fixes #29491 Extension install count comma formatting Fixes #29491 Jul 16, 2017
@cleidigh
Copy link
Contributor Author

cleidigh commented Jul 16, 2017

@sandy081
Just want to check if this one liner is okay to close out.
Cheers

@Tyriar
Copy link
Member

Tyriar commented Jul 21, 2017

Ping @sandy081, not sure if we want to do this for German (#29491 (comment)).

@cleidigh
Copy link
Contributor Author

@sandy081
Do you have an opinion on whether to use appropriate locale format or commas always?

@sandy081
Copy link
Member

@cleidigh Sorry for the delay.

Is there a reason why not using toLocaleString api?

@Hirse
Copy link
Member

Hirse commented Jul 24, 2017

Does toLocaleString require a locale in this case or would it default to the correct one?
It one is required, how would the correct locale be retrieved?

I put the original comment because this looked wrong to me:

installLabel = installCount.toLocaleString('en');

@cleidigh
Copy link
Contributor Author

@sandy081
No problem, I know you guys are busy with the multi-root stuff , great new addition!

@Hirse / @sandy081
My inclination was to use platform.locale thus using the correct number formatting for any locale.
I did the PR just using the English locale (always commas) given others comments on simplicity.
I can change it to:

installLabel = installCount.toLocaleString(platform.locale);

@sandy081
Copy link
Member

@cleidigh Yeah, using the locale makes sense to me. Can you please change it accordingly? Thanks.

@cleidigh cleidigh changed the title Extension install count comma formatting Fixes #29491 Extension install count, use current locale for number formatting Fixes #29491 Jul 25, 2017
@cleidigh
Copy link
Contributor Author

@sandy081
Done.
Sorry for the protracted discussion on a one liner !

@sandy081 sandy081 merged commit 25b919c into microsoft:master Jul 25, 2017
@sandy081
Copy link
Member

LGTM. Thanks for the PR.

@cleidigh cleidigh deleted the ext-dload-cnt-fmt/bug branch July 25, 2017 19:56
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants