Refactor/23149 money request page #23979
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@koko57 Thanks for pushing this ahead. You can now merge main, the RNAnimated has been updated in main |
|
Taking this. |
|
@marcaaron thanks for the review! 😊 I fixed all the code with comments without NAB label. |
|
@marcaaron @mountiny Can I start working on this followup PR? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@koko57 Yes please, thanks everyone for pushing this ahead, no need to wait with the follow ups |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.51-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.51-2 🚀
|
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.52-0 🚀
|
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.52-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
| } | ||
| setCurrentAmount((prevAmount) => { | ||
| setSelection((prevSelection) => getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length)); | ||
| return MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces); |
There was a problem hiding this comment.
👋 Coming from #23933
Cursor jumps to next position when we use ',' in request money when language is English
This happens because we were updating the cursor position using the non-comma stripped amount and only after that strip the comma from the amount.
We resolved this by stripping comma from the amount first and updating the selection after that, i.e
setCurrentAmount((prevAmount) => {
const strippedAmount = MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);
setSelection((prevSelection) => getNewSelection(prevSelection, prevAmount.length, strippedAmount.length));
return strippedAmount;
});
There was a problem hiding this comment.
I think that makes sense but it's not a regression from this PR. That logic was copied from src/pages/iou/steps/MoneyRequestAmount.js,
|
This issue was caused by change introduced by this PR. Issue: Web - App crashes when currency in url is changed to invalid currency code Steps for reproduction:
|
|
@sobitneupane I don't think this PR introduced the issue 🙂 We didn't validate the currency before these changes were introduced |
|
|
||
| useFocusEffect( | ||
| useCallback(() => { | ||
| focusTextInput(); |
There was a problem hiding this comment.
Coming from #22388, this has caused a small regression
We're focusing the input immediately after the page is focused. When this happens when the page starts navigating, it will cancel the transition animation and we just see an abrupt page change
It was later decided to use this solution whenever we need to autofocus a TexInput, which doesn't have this problem
There was a problem hiding this comment.
@eVoloshchak It was already implemented earlier
App/src/pages/iou/steps/MoneyRequestAmount.js
Lines 292 to 296 in e2694f5
There was a problem hiding this comment.
Oops, I see, thank you!
| ) : null} | ||
| <Button | ||
| success | ||
| style={[styles.w100, styles.mt5]} |
There was a problem hiding this comment.
Adding a mt5 here caused this issue.
There was a problem hiding this comment.
it wasn't added in this commit 😅 it was just moved from the old file without changes
App/src/pages/iou/steps/MoneyRequestAmount.js
Line 462 in e2694f5
|
|
||
| const iouType = lodashGet(route, 'params.iouType', ''); | ||
| const reportID = lodashGet(route, 'params.reportID', ''); | ||
| const isEditing = lodashGet(route, 'path', '').includes('amount'); |
There was a problem hiding this comment.
This didn't work when open deep link from chat message.
More details about the root cause: #26471 (comment)
So we update logic like this:
| const isEditing = lodashGet(route, 'path', '').includes('amount'); | |
| const isEditing = Navigation.getActiveRoute().includes('amount'); |
EDIT: the original logic was added here: https://github.com/Expensify/App/pull/17964/files#diff-879f243892ee617d3ea9bc2550496a07521c2b91d2bec3eb12b90f09aac6a05aR94
| }, [onSubmitButtonPress, currentAmount]); | ||
|
|
||
| const formattedAmount = MoneyRequestUtils.replaceAllDigits(currentAmount, toLocaleDigit); | ||
| const buttonText = isEditing ? translate('common.save') : translate('common.next'); |
There was a problem hiding this comment.
This component is used in EditRequestAmountPage as well. isEditing prop is not passed so it caused #33011
There was a problem hiding this comment.
@situchan this prop was passed properly in EditRequestAmountPage, but it was lost during refactor https://github.com/Expensify/App/pull/28618/files#diff-598c08eb72e9071a7ab827304a24f3dd0a769b65f71e9c30140df3eda8c4b667
Details
Fixed Issues
$ #23149
PROPOSAL: #23149(comment)
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-08-01.at.09.42.00.mp4
Mobile Web - Chrome
Screen.Recording.2023-08-01.at.11.53.10.mp4
Mobile Web - Safari
iphone_safari.mov
Desktop
Screen.Recording.2023-08-01.at.11.54.47.mp4
iOS
iphone_app.mov
Android
Screen.Recording.2023-08-01.at.11.41.32.mp4