Add asyncOpenURL wrapper #3764
Add asyncOpenURL wrapper #3764
Conversation
|
Slack thread for context: https://expensify.slack.com/archives/C01GTK53T8Q/p1623955103415500 |
| * @param {Promise} promise | ||
| */ | ||
| export default function asyncOpenURL(promise) { | ||
| const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent); |
There was a problem hiding this comment.
Can you explain why are you doing a negative patten match for chrome or android here? Is there some case when the user agent would be formatted like 'chrome-xxx-safari' or something?
There was a problem hiding this comment.
@roryabraham
The negative pattern match is to exclude chrome, edge, and android.
All of them like to include Safari in their UA
Edge and Chrome's userAgent: "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.114 Safari/537.36"
Also, I think all third party browsers on iOS are just skins for safari.
So chrome on iOS has this issue too. And this pattern detects that in my testing :)
Chrome on iOS detected as Safari
There was a problem hiding this comment.
Thanks for answering this @rushatgabhane! Yes, that is the reason I used the negative pattern match. @roryabraham Do we have any other preferred method?
| @@ -0,0 +1,10 @@ | |||
| import {Linking} from 'react-native'; | |||
There was a problem hiding this comment.
It's been a while so I'm not 100% sure, but does it work to provide an index.website.js for the web implementation and just index.js for all other platforms?
There was a problem hiding this comment.
@roryabraham It seems to work! Would that be preferred? If so, I will also go ahead and update the docs here. I only saw a handful of other modules where all of the files were used, but the code was actually unique to each platform.
| * | ||
| * @param {Promise} promise | ||
| */ | ||
| export default function asyncOpenURL(promise) { |
There was a problem hiding this comment.
Also, what do you think about just providing the url as an argument to this function? Is that simpler/less prescriptive than this implementation, in which the promise needs to resolve to a url? Basically something more like this implementation
|
Also, we should make sure to test on all platforms to ensure we don't introduce a regression on other platforms |
|
Updated with requested changes and tests on all platforms. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Unfortunately the deploy comments are not working right now. This was deployed to staging yesterday. |
cc @chiragsalian @roryabraham
Fixed Issues
$ #3615
Tests
Request Moneyrequest from Account A to Account B (both accounts must have PayPal.me username set inSettings > Payments)PayPal.meand then confirm the payment.QA Steps
Steps above.
Tested On
Screenshots
Web
Screen.Recording.2021-06-25.at.4.28.22.PM.mov
Mobile Web
web-mobile.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov