IOU - Localize amount text input #7221
Conversation
MelvinBot
requested review from
Luke9389 and
parasharrajat
and removed request for
a team
songlang1994
force-pushed
the
iou-amount-localization
branch
from
59b21ae to
55b2372
Compare
src/components/withLocalize.js
Outdated
| isValidLocaleDigit(digit) { | ||
| return LocaleDigitUtils.isValidLocaleDigit(this.props.preferredLocale, digit); | ||
| } | ||
|
|
||
| toLocaleDigit(digit) { | ||
| return LocaleDigitUtils.toLocaleDigit(this.props.preferredLocale, digit); | ||
| } | ||
|
|
||
| fromLocaleDigit(localeDigit) { | ||
| return LocaleDigitUtils.fromLocaleDigit(this.props.preferredLocale, localeDigit); | ||
| } | ||
|
|
There was a problem hiding this comment.
I can't think of any other place in the app that will use these functions so I don't think it is a good idea to pollute the WithLocalize context at this time. Maybe directly use them in the IOUAmountPage for now and pass the locale from this.props.preferredLocale. We can refactor these in future if needed.
There was a problem hiding this comment.
Currently toLocaleDigit is also used by BigNumberPad.
There was a problem hiding this comment.
Also, I cleaned up the isValidLocaleDigit function because it's not used. So only toLocaleDigit and fromLocaleDigit is added in that context.
songlang1994
changed the title
| } | ||
|
|
||
| render() { | ||
| const formattedAmount = this.replaceAllDigits(this.state.amount, this.props.toLocaleDigit); |
There was a problem hiding this comment.
Can't we just use, numberFormat prop directly for the whole input value?
There was a problem hiding this comment.
Unfortunately we can't. Because the amount might not be a valid number yet while inputing (e.g. "12.").
If we do need to use numberFormat the code will be
get formattedInputingAmount() {
if (!this.state.amount) {
return '';
}
const endsWithDecimalPoint = this.state.amount.endsWith('.');
if (endsWithDecimalPoint) {
const localized = this.props.numberFormat(this.state.amount, {useGrouping: false, minimumFractionDigits: 1});
return localized.slice(0, -1);
}
const fraction = this.state.amount.split('.')[1];
const fractionDigits = fraction ? fraction.length : 0;
return this.props.numberFormat(this.state.amount, {useGrouping: false, minimumFractionDigits: fractionDigits});
}There was a problem hiding this comment.
Ok. What is more performant?
There was a problem hiding this comment.
There is no performance issue here since the max length of amount is just 8. But replaceAllDigits here is more readable.
There was a problem hiding this comment.
Sounds, good. I will give it a go.
@parasharrajat That's the expected behavior of current implementation. Maybe you would like to double check it by asking a native Spanish user. |
|
ok. Thanks. I will ask @Luke9389 to test it and then decide. |
|
I think needing to type |
|
I think it'd be really frustrating for users to by typing one thing, and then have a totally different character show up. |
|
@songlang1994 You got some conflicts to resolve. |
|
Should I use |
|
Git merge and resolve conflicts manually. |
There was a problem hiding this comment.
Ok. LGTM. Good work @songlang1994 .
cc: @Luke9389
🎀 👀 🎀 C+ reviewed
There was a problem hiding this comment.
Nice! I was about to approve this yesterday but wanted to adhere to the Contributor+ structure. Great work both of you @songlang1994 @parasharrajat. 👍
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by @AndrewGable in version: 1.1.32-0 🚀
|
Details
IOU - Localize amount text input. Users will be able to type/paste localized amount format in amount input. See also: #6427 (comment)
Fixed Issues
$ #6427
Tests
QA Steps
Same as above.
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android