Make Mobile Web and Native App Scanning Consistent#28411
Make Mobile Web and Native App Scanning Consistent#28411AndrewGable merged 14 commits intoExpensify:mainfrom
Conversation
|
|
||
| /* Forwarded ref */ | ||
| forwardedRef: refPropTypes.isRequired, | ||
|
|
There was a problem hiding this comment.
Following the guidelines, it's necessary to add a description for all props
melvin-bot
bot
requested review from
jakub-trzebiatowski
and removed request for
a team
|
@cubuspl42 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] |
ios/Podfile.lock
Outdated
| - RNReactNativeHapticFeedback (1.14.0): | ||
| - React-Core | ||
| - RNReanimated (3.5.4): | ||
| - RNReanimated (3.4.0): |
| const navigation = useNavigation(); | ||
| const [isCameraActive, setIsCameraActive] = useState(navigation.isFocused()); | ||
|
|
||
| // Get the animation value from the tab navigator. Its a value between 0 and the |
|
|
||
| // Get the animation value from the tab navigator. Its a value between 0 and the | ||
| // number of pages we render in the tab navigator. When we even just slightly start to scroll to the camera page, | ||
| // (value is e.g. 0.001 on animation start) we want to activate the camera, so its as fast as possible active. |
There was a problem hiding this comment.
its -> it's
Also, the sentence structure in the comment isn't proper English, the comment could be passed through GPT or another tool with a prompt like "Improve language correctness"
| if (!cameraRef.current.getScreenshot) { | ||
| return; | ||
| } | ||
| const imageB64 = cameraRef.current.getScreenshot(); |
There was a problem hiding this comment.
Nit: imageB64 -> imageBase64 ?
|
| <View style={[styles.uploadReceiptView(isSmallScreenWidth)]}> | ||
| {!isDraggingOver ? ( | ||
| <View style={[styles.flex1, !Browser.isMobile() && styles.uploadReceiptView(isSmallScreenWidth)]}> | ||
| {!isDraggingOver && Browser.isMobile() && ( |
There was a problem hiding this comment.
This becomes a wall of text...
How about...
const buildPrimarySection= () => {
if (Browser.isMobile()) {
return buildMobilePrimarySection();
} else {
return buildDesktopPrimarySection();
}
}
// ...
{!isDraggingOver && buildPrimarySection()|This way, we highlight that this is a mobile/desktop switch, and one of them always renders.
The refactored fragments could be either local functions in this component (with no arguments), private functions in this module (with appropriate arguments introduced), or even new components, whichever you find preferable.
There was a problem hiding this comment.
made some amendments
Looks good! Please also check the other threads with minor comments.
|
@cubuspl42 made some amendments |
|
@lukemorawski We're 869 commits behind, let's merge |
|
@cubuspl42 merged, no conflicts |
new-web-scan-flow-android-web-compressed.mp4I don't know; maybe it's something on my side, but shouldn't I be prompted about the appropriate permission? |
|
@cubuspl42 are you running it in an emulator? |
|
Yes! Is it unsupported? |
|
ah, yes you are :) Then you need to add that address (with port number) to the white list: Video streaming works only on https connections |
|
@lukemorawski Any tricks for iOS Safari? :) Also, do you know any tricks for the spinning wheel on Native? Might also be emulator-related, and works same on Edit: Damn, it's just not existing. new-web-scan-flow-ios.mp4 |
This might be bit tricky, but I'll see what can be done. |
|
@JmillsExpensify Added some changing to styling, though that weird resize/flash is much more tricky to tackle. |
I actually think we might have done this on native when we saw a similar issue, specifically chancing the opacity? @AndrewGable would know for sure, one way or another. In any case, given that I'm one of the only ones seeing it, it could also have something to do with my account (I have a "heavy load" account with thousands of reports; on iPhone 14 Pro). All in all, I'm fine tackling this in a follow-up. |
|
Seeing as we're not considering @JmillsExpensify 's lag experience a blocker, and the padding issue is relatively straightforward, what is you best estimate at when this might be finished @lukemorawski ? Anything else in our way currently? |
|
@MitchExpensify padding is finished already. PR is ready to be re-reviewed. |
|
Kicked off another adhoc build to test this. |
|
FYI now consistently get the bug @JmillsExpensify was seeing 😢 - I think it might be a blocker if others see it too. RPReplay_Final1696969957.MP4 |
|
@AndrewGable I had a dream tonight about a possible solution and have one more thing to try :) If that won't work, then the only thing left is to show the camera view after few hundred ms, to cover this bug. |
|
@AndrewGable I think it's fixed! The solution is so ridiculously simple, though completely unexpected. Tested only on one iPhone device. |
|
Nice! I will re-build and test once more! |
|
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
AndrewGable
left a comment
AndrewGable
left a comment
There was a problem hiding this comment.
Looks great on my phone, nice fix! Just waiting on a few others to test then I will merge.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
FYI this PR caused this regression because ReceiptSelector is also used outside the tab navigator and the |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
8 participants
Details
This PR adds a direct camera view to the receipt scan flow on mobile web, much like in a native ios/android experience.
Button left to the camera shutter still allows to access photo library/files or to use the OS camera modal.
Button to the right can turn on the flashlight/torch, though it's availability depends on the browser capabilities (currently iOS Safari doesn't allow for this).
If open in a desktop environment, there still the previous attachment picker/drag'n'drop screen available.
Fixed Issues
$ #28204
PROPOSAL: no proposal
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
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
Web
Mobile Web - Chrome
Mobile Web - Safari
RPReplay_Final1696513318.mov
Desktop
iOS
Android