Conversation
This reverts commit 356f371.
|
Ok, I think the web issues are gone after merging main. Gonna look into the cross platform issue now |
|
I was able to test on iOS and I do see the image being updated on web. Android simulator is giving me a lot of headaches with the bad internet connection in Bali, so I'm not able to reliably test this condition. This PR is blocking others, so given that it's working on all platforms but android I think we should go ahead and merge this PR and then address the issue in a follow up if we encounter it on staging ios.web.mov |
Gonals
left a comment
There was a problem hiding this comment.
Code looks good! Just one question and I'll jump to testing
| }} | ||
| onModalHide={(e) => { | ||
| props.onModalHide(e); | ||
| if (onModalHideCallbackRef.current) { |
|
Tests well, but Android is also being a pain for me. I'm ok merging this |
|
I don't know what to do with this. For me it works very unpredictable and I'm not sure if I want to take responsibility for approving this... |
|
Another issue, this time on iOS: Screen.Recording.2023-09-19.at.08.37.37.mov |
|
@burczu could you clarify what you mean by:
Happy to help test here, if needed, @luacmartins. |
|
@MariaHCD I encountered some issues (you can see my comments in the thread above), that I don't see resolved. If you like to test it, go on - maybe it's the matter of my environment. Right now I'm struggling with Android build so I can't get back to this PR at the moment. |
|
@luacmartins I've cleaned my environment and re-tested everything with new user. Web is now working as expeced, as well as on Android. The only issue I have is with iOS - please see this comment. It sometimes work but in most cases it throws an error of "Unhandled Promise Rejection". |
|
That error points to this fetch call failing with a |
|
@MariaHCD I'd love another review here so we can get this merged asap. |
burczu
left a comment
There was a problem hiding this comment.
Alright, I was able to successfully replace the image on iOS so I think I can approve it... I'm not sure how improving error handling will fix the issue with rejecting this Promise, because I guess the question is why it is rejected not how it is handled...
|
Yea, that makes sense. I'll investigate that as part of the other issue! |
|
✋ 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 https://github.com/luacmartins in version: 1.3.72-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.72-11 🚀
|
| icon: Expensicons.Camera, | ||
| text: props.translate('common.replace'), | ||
| onSelected: () => { | ||
| onModalHideCallbackRef.current = () => Navigation.navigate(ROUTES.getEditRequestRoute(props.report.reportID, CONST.EDIT_REQUEST_FIELD.RECEIPT)); |
There was a problem hiding this comment.
This caused #27903 when navigating to the replace receipt screen not from within a tab navigator, i.e. whenever clicking from the three dots menu.
| <HeaderWithBackButton | ||
| title={translate('common.receipt')} | ||
| onBackButtonPress={Navigation.goBack} | ||
| /> | ||
| <DragAndDropProvider> | ||
| <ReceiptSelector | ||
| route={route} | ||
| transactionID={transactionID} | ||
| /> | ||
| </DragAndDropProvider> |
There was a problem hiding this comment.
This causes an inconsistency issue here where the Green drag and drop area does not show in full-screen #28014
Details
Adds the ability to replace transaction receipts
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/295248
Tests
ScanoptionOffline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
I'm having issues building iOS and android native at the moment. I'll try to upload the videos later.
Web
web.mov
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov