-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[core] feat(NumericInput): support localization #4388
[core] feat(NumericInput): support localization #4388
Conversation
Thanks for your interest in palantir/blueprint, @hunterxyz! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
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.
Mostly looks good, thanks for the PR @hunterxyz! I left some comments.
packages/docs-app/src/examples/core-examples/common/locales.tsx
Outdated
Show resolved
Hide resolved
Also this locales list is pretty long and unwieldy as shown here...
|
Since I reduced the list to 7 entries, a complex select is no more justified I guess, let me know |
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.
looks pretty good! just some minor comments
@hunterxyz I will cut a release of Blueprint tonight, would be great to get this PR included |
Oh my... I'm going to sleep in a bit, I'm in europe. |
@hunterxyz ah, ok then, no worries! no rush at all :) |
I pushed my suggested minor changes |
ok :D |
It's probably an issue with the docs setup, but it's possible to cause the linked docs build to crash with
|
@jlismore thanks for reporting, I can repro that bug in the docs app but it sounds like an unlikely scenario in real world usage... nonetheless would you mind filing a new issue to track the bug? |
Checklist
Changes proposed in this pull request:
This pull request is proposing to implement the locale to the NumericInput component and changing the format according to it.
Example:
en-US : 1000.99
de-DE: 1000,99
Reviewers should focus on:
number formatting flow
Screenshot