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

Change disconnect button disable logic to fix for MWA #773

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mcintyre94
Copy link
Collaborator

Instead of using wallet use connected, which also works for MWA

Previously we used !wallet, but wallet is defined in Android with MWA. This PR changes to use !connected so that the disconnect button is correctly disabled on Android when no wallet has been connected yet.

Before (current https://solana-labs.github.io/wallet-adapter/example/ on Android):
Screenshot_2023-04-21-11-12-31-19_40deb401b9ffe8e1df2f1cc5ba480b12

After:
Screenshot_2023-04-21-11-12-10-92_40deb401b9ffe8e1df2f1cc5ba480b12

Instead of using wallet use connected, which also works for MWA
@mcintyre94 mcintyre94 requested a review from jordaaash April 21, 2023 10:24
@changeset-bot
Copy link

changeset-bot bot commented Apr 21, 2023

🦋 Changeset detected

Latest commit: 87922af

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@solana/wallet-adapter-material-ui Patch
@solana/wallet-adapter-ant-design Patch
@solana/wallet-adapter-react-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jordaaash
Copy link
Collaborator

I'm not sure about this, but instinctually I suspect this change could lead to a weird state. If a wallet is selected, but that wallet becomes disconnected, it's possible that the wallet property will still be non-null, but the connected property will be false. This could lead to the disconnect button being displayed, but disabled, leaving the user stuck. Maybe you've already thought of this, but it's worth trying to rule out.

@mcintyre94
Copy link
Collaborator Author

I'm not that strongly opinionated about this, happy to just close it. But I guess in that case the multi button should still allow the user to disconnect if it's used. It'd either act as a connect button (if publicKey was unset), or have the disconnect in the dropdown (if publicKey was set). It looks like a connect button would also be enabled when wallet is non-null and connected is false: that button uses disabled={disabled || !wallet || connecting || connected}
But if an app was only displaying a disconnect button and connected was false, then I think you're right that that button would be disabled and the user would be stuck. I guess the problem is that there are a few different states the app could be relying on to decide to only display disconnect, so maybe this isn't worth messing with.

@jordaaash
Copy link
Collaborator

If it fixes the UX with MWA I think it's probably worth doing! I just want to make sure it doesn't break something else in the process.

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.

2 participants