New layout for Scan screen in landscape mode#87875
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 095c8a0cbe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…ad of takeSnapshot
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
|
@Expensify/design in case you want to test the new layout |
|
Videos seem good but will run a test build. |
|
🚧 @shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Oh yeah weird. I thought we added like a min-height to that receipt |
This was reported and is fixed on this PR |
|
Awesome, thanks for confirming! |
|
@GCyganek Could you merge main here? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-04-16.at.16.36.48.movAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-04-16.at.18.15.00.moviOS: mWeb SafariMacOS: Chrome / Safari |
|
@GCyganek In The current approach scatters isInLandscapeMode conditionals across 10+ places in the JSX tree (styles.flexRow, styles.permissionViewLandscape, getCameraViewfinderStyle(...), styles.pb4 : styles.pr4, horizontal={!isInLandscapeMode}, duplicated ReceiptPreviews, etc.), which makes the render logic hard to follow and increases the risk of accidentally breaking one orientation when editing the other. Have you considered using an early return pattern instead? |
Hmmm yeah maybe it would be cleaner here, haven't thought about this change after I finished. I think it's a good idea, will try in a moment |
…adjustments-landscape-mode
|
@truph01 I tried to do that separation based on |
I agree |
| }; | ||
|
|
||
| function ReceiptPreviews({submit, isMultiScanEnabled, isCapturingPhoto = false}: ReceiptPreviewsProps) { | ||
| function useReceiptPreviewsSizes(isInLandscapeMode: boolean) { |
There was a problem hiding this comment.
Methods and utils like this should have unit tests, can you add that in future pr?
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Explanation of Change
Changing layout of the Scan screen in landscape mode according to this and discussion here
Fixing #87318 by using
takePhotoinstead oftakeSnapshotin landscape mode to capture full imageFixing #87305 by showing full preview of the photo that will be taken
Fixed Issues
$ #77280
$ #87318
$ #87305
PROPOSAL: #85229 (comment)
Tests
Android:
iOS:
Make sure that Scan screen functionalities work fine in landscape mode with new layout
Offline 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-scan-new-layout.mp4
iOS: Native
scan-new-layout-ios.mp4