Add Download app promo banner on desktop upload expense flow #61885
Conversation
|
I've started a new Slack discussion here to confirm the translation |
|
@mananjadhav 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] |
|
Sorry if my issue was poorly explained, but I think we'd wanna show it on Desktop web as well: Here it's not showing 🤔 @jamesdeanexpensify @Expensify/design for visibility |
|
I’ve updated the PR to include desktop web as well |
|
Just so I'm clear - where are we going to add this? Web, mobile web, and desktop? I feel like we add it to everywhere but the mobile app, personally, but curious for thoughts! |
|
That makes sense, and I feel pretty strongly we shouldn't show this to users who already have the apps installed. |
|
Agreed! |
Agree, especially if there's no way to dismiss it. |
|
The reason why I didn't wanna push for mobile is the UI for mobile web is different:
The UI above leads little room for that banner unless we wanna rejigger stuff. Plus that sometimes iOS and Android show that "Download app" banner at the top of the page. |
|
Ah, fair! So just desktop and web web then? Or what do you call it when you are on a computer and go on the world wide web? Just web? |
|
Desktop app and desktop web 😄 I do like web web too though 😂❤️ |
| @@ -968,6 +970,24 @@ function IOURequestStepScan({ | |||
| receiptImageTopPosition={receiptImageTopPosition} | |||
| /> | |||
| )} | |||
| {platform !== CONST.PLATFORM.MOBILEWEB && !isMobile() && ( | |||
There was a problem hiding this comment.
@nyomanjyotisa It would be better to have this in platform specific files.
|
@nyomanjyotisa Can you please resolve the conflicts? |
|
PR updated and retested. Also, what is the final decision regarding the button styling here?
|
|
I like the small button, but that orphan on the copy is killing me. Maybe we can add a word or two so the sentence feels more balanced? "Scan receipts directly from your phone" or something? |
|
|
|
Here is the comparison for both copy
|
|
I think |
|
LGTM 👍 |
| @@ -977,6 +993,25 @@ function IOURequestStepScan({ | |||
| receiptImageTopPosition={receiptImageTopPosition} | |||
| /> | |||
| )} | |||
| {!isMobile() && isLastAppLoginLoaded && !hasInstalledApp && ( | |||
There was a problem hiding this comment.
With these many changes and checks, I think it makes sense to make it a resuable component.
|
PR updated |
src/hooks/useHasInstalledApp.ts
Outdated
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| const useHasInstalledApp = () => { | ||
| const [lastIOSLogin] = useOnyx(ONYXKEYS.NVP_LAST_ECASH_IOS_LOGIN, {canBeMissing: true}); |
There was a problem hiding this comment.
@nyomanjyotisa We should use const [lastIOSLogin, lastIOSLoginResult] = useOnyx(ONYXKEYS.NVP_LAST_ECASH_IOS_LOGIN, {canBeMissing: true}); and get rid of the setTimeout.
src/hooks/useHasInstalledApp.ts
Outdated
| const [isLastAppLoginLoaded, setIsLastAppLoginLoaded] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| const timer = setTimeout(() => { |
There was a problem hiding this comment.
As I mentioned earlier this isn't needed, we should rely on loaded/loading status from useOnyx hook.
There was a problem hiding this comment.
Can you post revised screenshots based on these changes? To esnure it doesn't have any flicker or something.
There was a problem hiding this comment.
Sure, I've updated the screenshots
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safarimweb-safari-hide-download-banner.movMacOS: Chrome / Safariweb-show-download-banner.mov
MacOS: Desktopdesktop-show-download-banner.mov |
|
Quick bump @puneetlath |
src/ONYXKEYS.ts
Outdated
| /** Timestamp of the last login on iOS */ | ||
| NVP_LAST_ECASH_IOS_LOGIN: 'nvp_lastECashIOSLogin', | ||
|
|
||
| /** Timestamp of the last login on Android */ | ||
| NVP_LAST_ECASH_ANDROID_LOGIN: 'nvp_lastECashAndroidLogin', |
There was a problem hiding this comment.
I think we should also add the lastAndroidLogin and lastiPhoneLogin nvps here. Since those are also for our mobile apps.
| @@ -977,6 +978,7 @@ function IOURequestStepScan({ | |||
| receiptImageTopPosition={receiptImageTopPosition} | |||
| /> | |||
| )} | |||
| {!isMobile() && <DownloadAppBanner />} | |||
There was a problem hiding this comment.
Generally, we should be avoiding using isMobile() checks and instead having a separate index.web.ts and index.ts file, no?
There was a problem hiding this comment.
@puneetlath Here I felt it was okay because we don't want to show on Mobile Web as well as Native app. Even if we did add index.web.ts, we'll need to add a check for isMobile to hide it on mWeb.
There was a problem hiding this comment.
Yes, in this case, we do need the isMobile checks for mWeb
There was a problem hiding this comment.
Ok that makes sense to me. Can we add a comment explaining why we are using it here?
src/hooks/useHasInstalledApp.ts
Outdated
| import {useOnyx} from 'react-native-onyx'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| const useHasInstalledApp = () => { |
There was a problem hiding this comment.
Just a suggestion to make this even more explicit to what it is.
| const useHasInstalledApp = () => { | |
| const useHasLoggedIntoMobileApp = () => { |
src/hooks/useHasInstalledApp.ts
Outdated
|
|
||
| const isLastAppLoginLoaded = lastIOSLoginResult.status !== 'loading' && lastAndroidLoginResult.status !== 'loading'; | ||
|
|
||
| const hasInstalledApp = !!lastIOSLogin || !!lastAndroidLogin; |
There was a problem hiding this comment.
| const hasInstalledApp = !!lastIOSLogin || !!lastAndroidLogin; | |
| const hasLoggedIntoMobileApp = !!lastIOSLogin || !!lastAndroidLogin; |
src/hooks/useHasInstalledApp.ts
Outdated
| const [lastIOSLogin, lastIOSLoginResult] = useOnyx(ONYXKEYS.NVP_LAST_ECASH_IOS_LOGIN, {canBeMissing: true}); | ||
| const [lastAndroidLogin, lastAndroidLoginResult] = useOnyx(ONYXKEYS.NVP_LAST_ECASH_ANDROID_LOGIN, {canBeMissing: true}); | ||
|
|
||
| const isLastAppLoginLoaded = lastIOSLoginResult.status !== 'loading' && lastAndroidLoginResult.status !== 'loading'; |
There was a problem hiding this comment.
| const isLastAppLoginLoaded = lastIOSLoginResult.status !== 'loading' && lastAndroidLoginResult.status !== 'loading'; | |
| const isLastMobileAppLoginLoaded = lastIOSLoginResult.status !== 'loading' && lastAndroidLoginResult.status !== 'loading'; |
|
@nyomanjyotisa Can you fix the conflicts? and also look at the comments. |
|
PR updated and retested |
There was a problem hiding this comment.
Thanks for the update @nyomanjyotisa. Test the latest change again.
@puneetlath All yours.
| @@ -977,6 +978,7 @@ function IOURequestStepScan({ | |||
| receiptImageTopPosition={receiptImageTopPosition} | |||
| /> | |||
| )} | |||
| {!isMobile() && <DownloadAppBanner />} | |||
There was a problem hiding this comment.
Ok that makes sense to me. Can we add a comment explaining why we are using it here?
| import {useOnyx} from 'react-native-onyx'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| const useHasLoggedIntoMobileApp = () => { |
There was a problem hiding this comment.
Can we add a comment explaining what this hook does? Like that it returns whether the user has ever logged into one of the Expensify mobile apps.
|
PR updated! |
|
✋ 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/puneetlath in version: 9.1.54-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.1.54-7 🚀
|
Explanation of Change
Fixed Issues
$ #61422
PROPOSAL: #61422 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Test 1
Download apppromo banner displayed correctlyTest 2
Download apppromo banner is not displayedPR 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))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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
User without the app installed / not logged in
MacOS-Chrome.mp4
User with the app installed / logged in
MacOS-Chrome-2.mp4
MacOS: Desktop
User without the app installed / not logged in
MacOS-Desktop.mp4
User with the app installed / logged in
MacOS-Desktop-2.mp4