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.

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

#151 - add logic to disable button while wallets are in detection process #230

Merged

Conversation

sebastianscatularo
Copy link
Collaborator

add logic to disable the button while wallets are in the detection process

depends on: XLabs/wallet-aggregator-sdk#9

@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet July 7, 2023 14:19 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview July 7, 2023 14:19 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet July 7, 2023 14:19 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview July 7, 2023 14:19 — with GitHub Actions Inactive
Copy link
Collaborator

@valentinoConti valentinoConti left a comment

Choose a reason for hiding this comment

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

Approved!

A comment just in case: I'm almost sure that I have seen on some chains the availableWallets variable be falsy but when pressing 'connect' some wallets still popped up to select;

maybe it would be a good idea to test every chain before merge to see if we are not preventing the connection for some chains 🙏🏼, and if that happens maybe after X minutes we can make the connect button available 🤔

@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet July 7, 2023 14:29 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview July 7, 2023 14:32 — with GitHub Actions Inactive
@sebastianscatularo
Copy link
Collaborator Author

sebastianscatularo commented Jul 7, 2023

I've tested, and for the cases where availableWallets is falsy (availableWallets.length === 0), the button connect is disabled, using walletsNotAvailable, which is also watching for source chain changes to reset it, so you could have not detected wallets on sui, but once you change to ethereum, you want to see if there was detected wallets for ethereun, that is true, and for each case, you have a message.

Let me know if you have any other doubt and then we could merge it

151-test.mov
Screen.Recording.2023-07-07.at.11.45.32.mov

@valentinoConti
Copy link
Collaborator

Yes, what I meant is, for example in your video on the Aptos blockchain, you seem to have 'Snap' installed and thats why its available, and below you have lot of other wallets that show "Install X wallet"

If you don't have Snap installed, instead of showing the user the ones that can be installed, we are not going to show him anything and just disabling the connect button, right??

I mean that's still better than current behaviour but it's just a thought 🙌🏼, I think can be merged 👍🏼

@sebastianscatularo
Copy link
Collaborator Author

oh, I see your point, but this behavior is for detection, if we detect a mechanism to connect a wallet, we are good to go, maybe the message could be better, something like "we do not detect a wallet o mechanism to connect to a wallet ensure you have a wallet", but I'll keep that for a future PR

@sebastianscatularo sebastianscatularo merged commit 6edcc35 into main Jul 7, 2023
10 checks passed
@sebastianscatularo sebastianscatularo deleted the bug/metamask-does-not-connect-at-first-attempt branch September 8, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants