Added maxLength to IOU Amount field#5192
Conversation
…maxlength # Conflicts: # src/pages/iou/steps/IOUAmountPage.js
| ref={el => this.textInput = el} | ||
| value={this.state.amount} | ||
| placeholder="0" | ||
| maxLength={CONST.IOU.AMOUNT_MAX_LENGTH} |
There was a problem hiding this comment.
Why are we passing this as a prop to TextInputAutoWidth? It doesn't seem like we will allow the setting a value that is larger anyway?
There was a problem hiding this comment.
Good point. I added the prop first and then the number pad change. Will remove.
There was a problem hiding this comment.
@marcaaron I checked again there are two functions updateAmount and updateAmountNumberPad.
We can do one of the three things:
- Retain my
maxLengthchange - Add the length change in
updateAmountalso - Add the length change in
updateAmountand call it insideupdateAmountNumberPadinstead of setting the state.
Please let me know which one suits better?
There was a problem hiding this comment.
For now, I've kept 1. and updated the PR.
There was a problem hiding this comment.
I would lean towards including it in the validateAmount() method (which both of these methods you mentioned use) and prevent setting state instead of using the maxLength on the TextInput. It's a controlled component so if we prevent setting state the value should not change?
There was a problem hiding this comment.
Okay, that worked. PR updated.
| const amount = `${prevState.amount}${key}`; | ||
| return this.validateAmount(amount) ? {amount: this.stripCommaFromAmount(amount)} : prevState; | ||
| }); | ||
| if (this.state.amount.length < CONST.IOU.AMOUNT_MAX_LENGTH) { |
There was a problem hiding this comment.
Use an early return please.
if (this.state.amount.length === CONST.IOU.AMOUNT_MAX_LENGTH) {
return;
}There was a problem hiding this comment.
Okay will update
| const amount = `${prevState.amount}${key}`; | ||
| return this.validateAmount(amount) ? {amount: this.stripCommaFromAmount(amount)} : prevState; | ||
| }); | ||
| if (this.state.amount.length < CONST.IOU.AMOUNT_MAX_LENGTH) { |
There was a problem hiding this comment.
Wouldn't this make the effective max length 13?
There was a problem hiding this comment.
If the current length is < 14 then allow updating the state, else skip it. I tested this. But wouldn't matter as based on @marcaaron's comment I'll be updating the condition and the early return.
|
PR updated |
|
✋ 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 staging by @marcaaron in version: 1.0.97-1 🚀
|
|
This issue is failing this PR #5241 |
|
@deetergp @marcaaron It seems the |
|
Length 12 also caused the issue so I've made it 11 and raised a PR. Reverting the pull will still have alignment issues. Length 12: Length 11 |
|
This has been deployed to production and is now subject to a 7-day regression period. |
|
🚀 Deployed to production by @francoisl in version: 1.0.98-1 🚀
|
6 participants
@deetergp Can you please review?
Details
maxLengthto amount field to avoid design issuesFixed Issues
$ #5115
Tests
QA Steps
Web/Desktop
Mobile Web/iOS/Android
Tested On
Screenshots
Web
web-max-length-amount.mov
Mobile Web
mweb-max-length-amount.mov
Desktop
desktop-max-length-amount.mov
iOS
ios-max-length-amount.mov
Android
android-max-length-amount.mov