-
Notifications
You must be signed in to change notification settings - Fork 0
Update the UI of WalletConnect related screens #1452
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Haven't gone into testing it, just a static review.
Regarding the organisation of the screens:
PolygonIDConnectionRequestandWalletConnectConnectionRequestare added toMainStackParams. This implies there are corresponding screens for them. But the screens are not added to theMainNavigator, so there's a risk of calling unknown screens (and crashing the app potentially).- I see you kept
ConnectionRequestScreenas the entrypoint whatever the protocol (Polygon ID or WalletConnect) which then redirect to eitherWalletCOnnectConnectionRequestScreenor the Polygon ID one, which then both redirect to the sameConnectionRequestScreenContent. Wouldn't be easier to have two entrypointsWalletConnect...ScreenandPolygonId...Screenwith their own params and logic and both reusingConnectionRequestScreenContentfor the UI? (assuming it can actually be shared, seeing how the wallet selection has some specific code for WalletConnect alone)
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.
Just a first review for the moment.
I need a bit more time to dive into WalletConnect, particularly with the selected wallet and required namespaces, etc.
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.
Can you remove the commented code?
src/pages/Requests/ConnectionRequestScreen/WalletConnectConnectionRequestScreen.tsx
Outdated
Show resolved
Hide resolved
src/pages/Requests/ConnectionRequestScreen/WalletConnectConnectionRequestScreen.tsx
Outdated
Show resolved
Hide resolved
src/pages/Requests/ConnectionRequestScreen/WalletConnectConnectionRequestScreen.tsx
Outdated
Show resolved
Hide resolved
| { | ||
| paddingBottom: insets.bottom, | ||
| paddingRight: insets.right, | ||
| paddingLeft: insets.left, |
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 have a <ScreenWrapper> that you can use as the first component and then put the PagerView inside.
ScreenWrapper set the safe areas itself (but can be customised) so you don't forget them ... like here where they are set only on the first Page, not the other ones.
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.
You should delete the padding based on the insets here. The safe areas is already covered by ScreenWrapper.
This <View> is actually not needed here, you can delete it. There's a ScrollView to wrap everything
| navigation.setOptions({ | ||
| title: 'Connection Request', | ||
| }) | ||
| } |
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.
Just an optimisation, it could be like this:
navigation.setOptions({
title: currentPage === PageType.SelectWallet ? 'Select a Wallet' : 'Connection Request',
})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 will have to overwrite the close button here, for two reasons:
- For the WalletConnect protocol there is a "decline"/"rejecte" callback. If we close the modal with the default close from the common header, it just navigate back to the previous screen without rejecting the WalletConnect request.
- As we have several pages in this screen (request, wallet selection and result), I found myself clicking the close button in the wallet selection page while I simply wanted to go back to the request page.
A simple solution would be to remove the close button in the header and therefore be forced to use the Buttons in the bottom bar.
But it's not enough, as there's also the gestures (on iOS sliding down the modal to close, on Android the Back gesture), so two possibilities:
- either keep the gesture but set a navigation listener to "decline" the request when closing the screen
- disable the gestures to prevent (maybe the prefered way)
In both cases, see https://reactnavigation.org/docs/preventing-going-back/ and https://reactnavigation.org/docs/preventing-going-back/#limitations
Side note: it may be a good thing to implement this "prevent going back" and disable the gestures on any screen processing something (like Polygon ID that can take a few seconds to receive a credential). Shoudl create a ticket about 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.
Precisions:
-
A) If we disable the gesture totally, we can just simply, and actually have to, remove the "close" button from the header.
-
B) If we keep the gesture with a listener to decline the request if closing the screen, we should keep the close button in the first page (displaying the request) but override it as a back button in the wallet selection (preventing closing the screen) and disable the gesture in the result as long as it's processing, and re-enable when it's a success/failure
B) is the most exhaustive UX, but A) is less complex to implement.
| { | ||
| paddingBottom: insets.bottom, | ||
| paddingRight: insets.right, | ||
| paddingLeft: insets.left, |
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.
You should delete the padding based on the insets here. The safe areas is already covered by ScreenWrapper.
This <View> is actually not needed here, you can delete it. There's a ScrollView to wrap everything
| /> | ||
| </View> | ||
| <View key={`ConnectionRequestResult`}> | ||
| <View style={styles.container}> |
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.
This "ConnectRequestResul" page doesn't have proper horizontal spacing (padding). It should be defined in the styles.container but be careful as styles.container is used by another View somewhere else, so best to create a dedicated style.
| /> | ||
| </View> | ||
| <BottomActionBar | ||
| alertType='error' |
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 alertType is used for when displaying a alert in the bottom bar, so paired with the alertContent. As there is no alert needed, alertContent is not defined so alertType is unnecessary. You can remove it.
| )} | ||
| </ScrollView> | ||
| <BottomActionBar | ||
| alertType='error' |
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 alertType is used for when displaying a alert in the bottom bar, so paired with the alertContent. As there is no alert needed, alertContent is not defined so alertType is unnecessary. You can remove it.
💭 What's changed?
🧪 How to test these changes?
😨 Anything to be aware of?