-
Notifications
You must be signed in to change notification settings - Fork 220
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
🐛 ✨ [react-i18n][i18n.unformatNumber] handle negative numbers #1758
Conversation
f7902fd
to
25ef542
Compare
@@ -615,8 +615,11 @@ function normalizedNumber( | |||
.substring(lastDecimalIndex + 1) | |||
.replace(nonDigits, ''); | |||
|
|||
const isNegative = input.trim().startsWith('-'); |
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'm not sure how much we care about this, but some locales may use a different character as their "negative sign". Possibilities include:
- U+002D
hyphen-minus
- U+2212
minus
- U+FE63
small hyphen-minus
- U+FF0D
full-width hyphen-minus
Note, also, that some locales (e.g. ja-JP
) may use either narrow or wide versions of the minus sign.
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.
Oh interesting, I'll experiment with that a bit.
Regarding ja-JP
and the narrow/wide characters, I don't think that unformatNumber
handles wide versions of 0-9 since they don't fall into the non-digit /\D/g
regex. Example
Along the same lines I was reading this number formatting guide from MS: https://docs.microsoft.com/en-us/globalization/locale/number-formatting
The way negative numbers are displayed.
The negative sign can be used at the beginning of the number, but it can also be used at the end of the number. Alternatively, the number can be displayed with parentheses around it or even in a color such as red. Thus a negative five hundred and twenty-seven could be displayed as:
-527
527-
(527)
[527]
It mentions how the negative sign could be at the start or end of the string as well as different ways of representing negative numbers.. but I don't feel like it wouldn't be practical (or reasonable maybe) to handle all those.
Thoughts?
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'm a big fan of incremental improvements; what you have here now is better than what we had before, so it's worth shipping (assuming @ismail-syed is OK with the style).
Personally, I'd be a fan of circling back with a second PR to handle alternative minus signs at the beginning of the number.
IMHO postfix minus signs, and parentheses to indicate negativity, are too esoteric and not worth supporting.
But others may have other opinions. :)
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.
Sounds good to me! Thanks @cejaekl
cdb4e15
to
6c2a4cc
Compare
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.
Fixup the failing test, but other than that, looks good to me. when CI is green
- make i18n.numberSymbols() public - document unformatNumber/unformatCurrency/numberSymbols
to make sure it doesn't overlap with expected value
40b25e8
to
a2bb093
Compare
Description
Fixes bug with
i18n.unformatNumber()
stripping the negative sign-
from negative numbers.Exposes
i18n.numberSymbols()
as a public method.Document previously undocumented
unformatNumber
/unformatCurrency
methods.Reference slack thread: https://shopify.slack.com/archives/C7TJQLVC7/p1614274001087600
Type of change
react-i18n
Minor: New feature (non-breaking change which adds functionality)Checklist