Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[uni fix] connect and switch network (network selector) #406

Merged
merged 9 commits into from
Apr 18, 2022
Merged

[uni fix] connect and switch network (network selector) #406

merged 9 commits into from
Apr 18, 2022

Conversation

W3stside
Copy link
Contributor

@W3stside W3stside commented Apr 13, 2022 •
edited
Loading

Summary

This is a 3FER!

Closes #340, closes #352, and closes #341 + 2 minor other broken things

Testing

  • disconnect wallet and switch wallet to rinkeby
  • using network selector connect to RINKEBY or GCHAIN
  • should toggle wallet, switch to network
  • check url
  • switch url chain id

@W3stside W3stside requested review from a team April 13, 2022 17:24
@github-actions
Copy link
Contributor

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@elena-zh
Copy link

elena-zh commented Apr 14, 2022 •
edited
Loading

Hey @W3stside , great job!

I have found several nitpicks and tricky issues, but let me know please if I need to report a separate issue for them:

  1. Related to 2440/uni merge: remove 'in the dropdown menu' from the Wrong Network message #352: 'in the dropdown menu' is still displayed in the warning message. However I noticed the changes logic that selecting a network in the dropdown calls this modal. But actually, it does not resolve the issue itself: a network is not changed in the wallet. So I still vote for removing this phrase from the modal.

  2. Network selector is enabled when connected to WC wallets (as an example, to Safe). So when I try to change a network, I see this warning message, that seems to me illogical as Safe wallet does not support networks changing. So can we disable this dropdown when connected to SC wallets at least?
    Screenshot_2

  3. When I connect to MM using WC in a mobile device, I'm able to change networks using dropdown. But after approving this request, I can face the message to open another app, that navigates me to the MM in the Appstore.
    See the video (in this video you can also see that I can't open the dropdown for a while after networks changing): https://drive.google.com/file/d/1jR5AuVWL0O-PMRuIPdQod5uc7AGR-Hm4/view?usp=sharing

Thanks!

alfetopito
alfetopito reviewed Apr 14, 2022
View reviewed changes
src/custom/constants/tokens/tokensMod.ts Show resolved Hide resolved
src/custom/hooks/useApproveCallback/useApproveCallbackMod.ts Show resolved Hide resolved
src/custom/hooks/useChangeNetworks.ts Outdated Show resolved Hide resolved
src/custom/hooks/useChangeNetworks.ts Outdated Show resolved Hide resolved
src/custom/pages/App/index.tsx
@@ -89,9 +88,6 @@ function createRedirectExternal(url: string) {
const Loading = <LoadingWrapper>Loading...</LoadingWrapper>

export default function App() {
// Dealing with empty URL queryParameters
useFilterEmptyQueryParams()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it break anything else in the app?
I mean, it must have had a reason to be added, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nyehhh

Base automatically changed from 2440/uni-merge to develop April 14, 2022 14:57
@alfetopito
Copy link
Collaborator

One thing that this PR is breaking is the ability to switch networks without being connected, which was possible before.
I think that's a great feature and we should keep that.

@W3stside W3stside mentioned this pull request Apr 18, 2022
Closed
@W3stside
Copy link
Contributor Author

Merging this. We now have #412 and should keep it in mind

@W3stside W3stside merged commit d995ca8 into develop Apr 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2022
@alfetopito alfetopito deleted the 2440/uni-merge-fixes-pt-2 branch April 19, 2022 08:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alfetopito alfetopito alfetopito left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@W3stside @elena-zh @alfetopito