-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Add placeholder thumbnail to expenses with no receipt #52848
Conversation
@gijoe0295 I've noticed an issue: clicking a placeholder receipt in the request preview doesn’t redirect to the expense, unlike clicking a regular receipt preview: 52848_bug.mov |
👋 @gijoe0295 Have you had a chance to look at the latest comments? Thanks. |
Yeah I found the root cause. Will push the changes soon. |
@brunovjk @dannymcclain I fixed the uneven borders & navigation press issues. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry @gijoe0295, when it's ready for review, please let me know. Thank you :D |
@brunovjk Sorry for the confusion. It's ready for review. I just forgot to push the last commit for fixing the onPress issue. |
No worries 👊 |
Reviewer Checklist
Screenshots/VideosAndroid: Native52848_android_native.movAndroid: mWeb Chrome52848_android_web.moviOS: Native52848_ios_native.moviOS: mWeb Safari52848_ios_web.movMacOS: Chrome / Safari52848_web_chrome.movMacOS: Desktop52848_web_desktop.mov |
This comment was marked as outdated.
This comment was marked as outdated.
Looking good @gijoe0295, except for one detail: We need to show the receipt instead of the Add receipt placeholder after the user uploads a receipt. Let me know your thoughts. Thanks a lot. Productionprod.movThis PRpr_bug.mov |
@gijoe0295, did you have a chance to take a look at my last comment? Thanks. |
@brunovjk I resolved it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed some unrelated issues, like warnings in native and bugs like duplicate preview creation or the scan confirmation modal not disappearing on desktop (though the expense is created). These were reported in Slack and are unrelated to this PR.
We need confirmation @Expensify/design. Other than that, everything seems good to me.
Additionally, @jasperhuangg, could you confirm if unit tests are required, should we include them here or address them in a separate PR? Thank you.
Running a build to test this bad boy |
🚧 @dannymcclain has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Quick spin and it looks pretty good to me! |
Looking good to me too! |
✋ 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/jasperhuangg in version: 9.0.71-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.0.71-2 🚀
|
Explanation of Change
Fixed Issues
$ #52638
PROPOSAL: #52638 (comment)
Tests
Offline tests
QA Steps
+1
in this case)PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop