-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Adds navigation and styled wrapper for webview - Introduces Error handling start #30027
Adds navigation and styled wrapper for webview - Introduces Error handling start #30027
Conversation
Flow as it stands with these changes: I based the navigation off of this pattern where we're using the WebView for the WalletStatementPage: |
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.
Couple of minor comments but this looks great so far. Noticed some weird behavior (with and without calling Sessions.clearSignInData()
when going back to the home page, definitely something to investigate further.
It might be nice to use the same interstitial loading view as we do for web/desktop. Right now that view lives in src/pages/signin/SAMLSignInPage/index.js
but maybe we could separate it out into it's own component like the DeeplinkRedirectLoadingIndicator
> | ||
<HeaderWithBackButton | ||
title="SAML Sign In" | ||
onBackButtonPress={() => 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.
I think here we'd want to call Session.clearSignInData();
here. Otherwise, if the user has SAML required on their account they'll be dropped right back into the same flow
includeSafeAreaPaddingBottom={false} | ||
> | ||
<HeaderWithBackButton | ||
title="SAML Sign In" |
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.
If we can leave this blank I think it'd be cleaner
@@ -19,39 +26,61 @@ const defaultProps = { | |||
credentials: {}, | |||
}; | |||
|
|||
const renderLoading = () => <FullScreenLoadingIndicator />; | |||
// const renderLoading = () => <FullScreenLoadingIndicator />; |
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.
Let's remove this if it's unused with the new logic
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.
one other small thing noticed while testing
/> | ||
<ScreenWrapper | ||
shouldShowOfflineIndicator={false} | ||
includeSafeAreaPaddingBottom={false} |
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'll need to add a component display name and then pass it here
includeSafeAreaPaddingBottom={false} | |
includeSafeAreaPaddingBottom={false} | |
testID={SAMLSignInPage.displayName} |
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.
Last one sorry 😅
const handleNavigationStateChange = useCallback( | ||
({type, url}) => { | ||
const searchParams = new URLSearchParams(new URL(url).search); | ||
if (searchParams.has('shortLivedAuthToken')) { | ||
const shortLivedAuthToken = searchParams.get('shortLivedAuthToken'); | ||
Session.signInWithShortLivedAuthToken(credentials.login, shortLivedAuthToken); | ||
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.
I don't think we need the return;
or Navigation.navigate(ROUTES.HOME)
here actually. Session.signInWithShortLivedAuthToken
should take care of any necessary redirects
Merged this logic into #29526 so we should be all set for this PR 👍 |
Details
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop