IOU - Localize amount text input#7221
IOU - Localize amount text input#7221Luke9389 merged 7 commits intoExpensify:mainfrom songlang1994:iou-amount-localization
Conversation
MelvinBot
requested review from
Luke9389 and
parasharrajat
and removed request for
a team
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
left a comment
parasharrajat
left a comment
There was a problem hiding this comment.
Code Looks good.
But when I selected Spanish, I have to type , on my keyboard. Is this behavior correct? or should it be like.. you type . and , is shown to the user when the selected language is Spanish.
@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. |
parasharrajat
left a comment
parasharrajat
left a comment
There was a problem hiding this comment.
Ok. LGTM. Good work @songlang1994 .
cc: @Luke9389
🎀 👀 🎀 C+ reviewed
Luke9389
left a comment
Luke9389
left a comment
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 🚀
|
4 participants
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