fix: error message if next button is tapped while a photo with flash#81252
fix: error message if next button is tapped while a photo with flash#81252dangrous merged 2 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppBefore fix: before.mp4After fix: after.mp4Android: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Please fix lint |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
trjExpensify
left a comment
There was a problem hiding this comment.
@Expensify/design are you happy with this? I kinda' feel like this solution 1) makes the app feel slow and 2) doesn't really provide good feedback on what's happening.
I would think it would be better UX to have the camera button provide that feedback if we're waiting for the picture to be taken coz' of the delay with flash.
|
We could do a little spinner in the shutter but I wonder if that'll just be even more jarring. I tested on my iOS phone and when you use the flash there they don't give any extra indication and neither does the Pixel phone I have. I kinda think you don't really run into this issue unless you spam for testing purposes. I don't mind this too much when you consider is in context. Curious for the others take though |
|
Do we have any videos of the proposed changes here? Otherwise I tend to agree with Jon. |
|
I will likely feel the same as Jon/Shawn, but yeah it'd be nice to see a video if possible! |
|
@daledah please update screen recordings to demonstrate that bug is fixed. Your current attachments don't make sense. |
|
Can someone please generate adhoc build? |
|
Running build 👍 |
|
🚧 @dubielzyk-expensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
I only watched the one Android video that was supplied in the PR OP: android.movTBF, I just tried to reproduce the bug reported on staging iOS a bunch of times and I couldn't at all. 🤔 🙃 |
|
Yeah, I'm pretty okay with this as I also tried to test it and it's not slow enough where you end up with a huge lag often imo. |
|
Cool, if you guys are happy... I guess I am! I am concerned that I can't repro the initial bug though. Do we even need to do this? |
|
Heh, if no one can reliably reproduce, I would love to do nothing. |
|
Yeah let's close if we can't repro. |
|
@daledah please reopen this. I am still able to reproduce |
|
@aimane-chnaif please move forward |
|
@dangrous please generate adhoc build |
|
Running! |
|
🚧 @dangrous has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Confirmed fix. All good to merge 🚀 |
|
🚧 @dangrous has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ 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/dangrous in version: 9.3.21-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.21-4 🚀
|
Explanation of Change
Fixed Issues
$ #80887
PROPOSAL: #80887 (comment)
Tests
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
android.mov
Android: mWeb Chrome
am.mov
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari