-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix App show error when open unlink in Desktop App #23738
Conversation
@Santhosh-Sellavel 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] |
@hoangzinh I find the Desktop steps confusing.
We won't show open in the desktop prompt for the dev environment. Have you checked by removing the restriction does it still work? |
src/ROUTES.js
Outdated
|
||
ROUTES_REGEX: { | ||
UNLINK_LOGIN: /\/u($|(\/\/*))/, | ||
}, |
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.
Is it okay to be here? I think its better to be on CONST
Thoughts @francoisl?
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.
Yeah agreed. Looks like we even already have a REGEX
object, which has a similar ROUTES
sub-object here. That sounds like a good place to put this.
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.
I updated PR to move this const to CONST
@Santhosh-Sellavel definitely I did. In my recording, I also tried to show it show the desktop prompt for the home url |
Will get to this tomorrow, please get this done @hoangzinh |
// Function to detect if current route should open in Web only. | ||
isUnsupportedDeeplinkRoute() { |
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.
Comment can be more user friendly
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.
Do you have any suggestion @Santhosh-Sellavel I think current comment is clear to me.
Seems to be tested well, After clicking the link we are redirected to the home page with a success message, but the unlink page is shown for a while after opening the link for unlinking is it a problem? i.e This page is shown for a while cc: @hoangzinh @francoisl |
@Santhosh-Sellavel It's not good UX, I think. What is the expected behavior here?:
|
@francoisl @NikkiWines What can we do here? |
Some sort of loading indicator sounds best to me (whether full page or not). We already set |
// A function to detect if current route should be opened in Web only. | ||
isUnsupportedDeeplinkRoute() { | ||
return _.some([CONST.REGEX.ROUTES.UNLINK_LOGIN], (unsupportRouteRegex) => { | ||
const routeRegex = new RegExp(unsupportRouteRegex); | ||
return routeRegex.test(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.
The method name looks sufficient. Instead, we could have a comment inside explaining why the unlink login path was added here. Thoughts?
cc: @hoangzinh @francoisl
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.
(Personal preference) I don't mind the comment outside the function, even if the name sounds sufficient. But overall neutral, I'm ok if we keep it or remove it.
However, yes a comment inside explaining why the unlink route is there sounds like a good idea to me!
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.
I added the comment inside this func. Please help me review it again. Thanks
@Santhosh-Sellavel @francoisl I just updated with the full page loading, as a simplest implement. Because if we go with not full page loading, we need to involve Designer/UX into and discuss where should we display loading indicator. I also recorded this full page loading in the Web section of this PR description. |
Reviewer Checklist
Screenshots/VideosWeb & DesktopUnlink.movMobile Web - ChromeScreen.Recording.2023-08-08.at.12.01.25.AM.moviOSSimulator.Screen.Recording.-.iPhone.14.-.2023-08-08.at.00.13.57.mp4AndroidScreen.Recording.2023-08-10.at.8.14.58.AM.mov |
Please resolve conflicts @hoangzinh! |
bump @hoangzinh! |
@Santhosh-Sellavel sorry I was off yesterday. I just resolved conflict of this PR. Could you help to review it again? Thanks |
const isUnsupportedDeeplinkRoute = useMemo( | ||
() => | ||
// According to the design, we don't support unlink in Desktop app https://github.com/Expensify/App/issues/19681#issuecomment-1610353099 | ||
_.some([CONST.REGEX.ROUTES.UNLINK_LOGIN], (unsupportRouteRegex) => { | ||
const routeRegex = new RegExp(unsupportRouteRegex); | ||
return routeRegex.test(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.
Why do we need to memorize this?
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.
I think it's an expensive calculator (loop through array and do regex) so I prefer memo it to prevent it rerun every time the component re-render.
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.
1 value in array I don't think its expensive. Let's just have a definition and use it.
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.
I just updated the PR. Please help me review it again. Thanks
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!
@francoisl bump! |
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 pushing this through to the end.
✋ 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/francoisl in version: 1.3.54-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/francoisl in version: 1.3.54-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.54-13 🚀
|
return; | ||
} | ||
|
||
Navigation.navigate(ROUTES.HOME); |
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.
Details
Fixed Issues
$ #19681
PROPOSAL: #19681 (comment)
Tests
On Web/mWeb
On Desktop
Ensure you logged out Expensify account in Web browser
On Native apps
Ensure you logged out Expensify account in Web browser
Offline tests
Can not test when offline
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screen.Recording.2023-08-04.at.09.06.42.-.web.mp4
Mobile Web - Chrome
Screen.Recording.2023-07-27.at.23.04.03.-.android.chrome.mov
Mobile Web - Safari
Screen.Recording.2023-07-27.at.22.53.01.-.ios.safari.mov
Desktop
Screen.Recording.2023-07-27.at.22.48.17.-.desktop.mp4
iOS
Screen.Recording.2023-07-27.at.23.02.10.-.ios.mp4
Android
Screen.Recording.2023-07-27.at.23.05.56.-.android.mp4