-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Desktop deeplink #13606
Desktop deeplink #13606
Conversation
@NikkiWines @Santhosh-Sellavel One of you needs to 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] |
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
@cowboycito commits are not signed So you need to retroactively sign commits on this branch. We are unable to merge pull requests which contain any unsigned commits. I think that the easiest way to do that would be to follow the instructions in this article. |
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.
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.
Did a HL once-over, will give this a proper review next week
26a5b14
to
1425f23
Compare
1425f23
to
b1b9ba6
Compare
Fixed all the issues reported by @NikkiWines, signed all the commits (cc @Santhosh-Sellavel) and applied the Spanish translations provided by @aldo-expensify. |
For this one, I would just use our current ExpensifyNeue font for the headline. We will follow up separately with a PR to add in the correct headline font, but no need to worry about it here. |
Done! |
Great! Mind updating the screenshots when you get a moment? Thanks! |
Thanks @cowboycito Can you update screen recording for other platforms too, Let's ensure that it's working as expected and no weird behaviors. |
@Santhosh-Sellavel sorry for the delay. Here are the recordings with the style adjustments. Chrome: chrome.movSafari (showing when the deeplink check works and when it doesn't): safari.movIs there anything else you want me to do so we can start reviewing the code? Also, what is the CLA check that is failing in the PR? How do I fix that? |
Looking good! cc @luacmartins as we'll want to update the headline font for this screen too. So depending on whether or not this gets merged before or after your fonts PR, we can adjust accordingly. |
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.
LGTM thanks for the changes!
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.
Looks great !! Thanks for your perseverance here @cowboycito & @Santhosh-Sellavel 🙌
One minor note for the testing steps @cowboycito :
Please notice that if you're testing in Safari there's a small bug that was already discussed #10893 (comment).
Might be good to add a bit more context here as to what the small bug is, so you don't have to go to a separate page to know if you've encountered the bug while testing.
@NikkiWines done |
@cowboycito seems like the author checklist is outdated. Could you please update it with the most recent version? |
@luacmartins done! |
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.
Working fine for me!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Started a discussion about the performance regression here |
@luacmartins let me know it I can/need to do anything as I don't have access to that Slack link |
Seems like we don't care about the meaningless changes to duration, so I'll remove the label and work on a fix to prevent adding the label in such cases. |
🚀 Deployed to staging by @NikkiWines in version: 1.2.52-0 🚀
|
🚀 Deployed to production by @Julesssss in version: 1.2.52-4 🚀
|
Sorry everyone, I believe this is not working as expected, @cowboycito cc: @NikkiWines |
@Santhosh-Sellavel check out this thread |
I'll dm you. |
@Santhosh-Sellavel screenshots of both staging and production after removing all references to Electron in any |
Ya, that helps, thanks @cowboycito. I still feel it's quite annoying for developers here. |
|
||
openRouteInDesktopApp() { | ||
const expensifyUrl = new URL(CONFIG.EXPENSIFY.NEW_EXPENSIFY_URL); | ||
const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}${window.location.pathname}`; |
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.
We forgot to pass the query params here which caused a few URLs to break in our App.
Here is the regression issue #15059
Hi all, from the issue the plan was that clicking any link would deeplink with desktop. Cheers! |
} | ||
} | ||
|
||
isMacOSWeb() { |
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.
Why did we put this in the component instead of the Browser
lib?
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.
No specific reasons we just missed out on that Review.
Details
Implemented deeplink on the web app that will show a popup to the user asking if they want to open the desktop app (if the desktop app is installed).
The deeplink code will trigger the popup if the user opens up the web app in any of the following routes:
Fixed Issues
$ #10893
PROPOSAL: #10893 (comment)
Tests
Launching Expensify
and the deeplink popup above it.Offline tests
Launching Expensify
and the deeplink popup above it.QA Steps
Launching Expensify
and the deeplink popup above it.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Chrome
Safari
Microsoft Edge
Firefox
Opera
iOS
ios.mov
iOS web
ios_mweb.mp4
Android web
android.mweb.mp4
Desktop
desktop.mp4