connect icon indicating copy to clipboard operation
connect copied to clipboard

feat: show other wallet options

Open Imamah-Zafar opened this issue 2 years ago • 5 comments

Description

The Connect UI currently only shows Hiro Wallet as the wallet to connect with if no wallet installation is detected. Since Hiro Wallet is not on mobile, users should be shown option to connect with Xverse, to retain mobile users. The UI is updated to add Xverse in the wallet options to connect with. On Desktop: Screenshot 2022-08-29 at 5 27 42 PM

Screenshot 2022-08-29 at 5 27 28 PM

On Mobile Screenshot 2022-08-29 at 5 41 52 PM

For details refer to issue #266

Type of Change

  • [ ] New feature
  • [ ] Bug fix
  • [ ] API reference/documentation update
  • [x] Other

Are documentation updates required?

No

Testing information

The initial wallet connect flow needs to be tested on supported browsers such as Chrome and Firefox as well as on unsupported browser like Safari. The wallet connect flow also needs to be tested on mobile

Imamah-Zafar avatar Aug 23 '22 07:08 Imamah-Zafar

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Imamah Zafar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Aug 23 '22 07:08 CLAassistant

Thanks for this PR! It's great to see movement on this front.

Some design suggestions:

  • Retain the illustrations (seen on https://github.com/hirosystems/connect/issues/266) somehow since they're helpful for exciting and informing the user, even if reduced in size to accommodate the new format
  • "Add a Wallet to Chrome" / "Add a Wallet to your mobile" -> "Get wallet to use [APP NAME]"
  • "...you will need a Stacks compatible wallet" -> "...you will need a Stacks-compatible wallet"
  • "Browser Extension" -> "Browser extension"
  • "Mobile Application" -> "Mobile application"
  • Remove "Choose a wallet" subheader since unnecessary
  • "Download it on your device.." -> "Install it on your device..."

@eugeniadigon wdyt?

markmhendrickson avatar Aug 23 '22 11:08 markmhendrickson

  • Retain the illustrations (seen on Show other wallet options #266) somehow since they're helpful for exciting and informing the user, even if reduced in size to accommodate the new format

Hi! All the feedback have been taken into account. Regarding the illustrations, it will be an issue on mobile for both dialogs that will be too long on smaller devices (for example on iPhone SE). Screenshot 2022-08-24 at 11 56 09

If it is mandatory to keep the illustrations, we will have to scroll inside the modal which isn't an optimal behavior. What do you think?

Thanks,

QUDBS avatar Aug 24 '22 09:08 QUDBS

The illustration can/should be removed as this should be wallet/brand-agnostic. Thanks!

ginny-d avatar Aug 24 '22 10:08 ginny-d

The design changes suggested have been implemented. the screenshots in the description have also been updated

Imamah-Zafar avatar Aug 29 '22 12:08 Imamah-Zafar

The latest commit has the changes suggested to remove repetitive mobile conditional check in styling

Imamah-Zafar avatar Sep 06 '22 05:09 Imamah-Zafar

Is there anything holding up this PR from release?

yknl avatar Sep 15 '22 06:09 yknl

👋 Great PR, thanks for the contribution — we do currently require signing a CLA for legal reasons. Should be possible via CLA Assistant

janniks avatar Sep 15 '22 14:09 janniks

👋 Great PR, thanks for the contribution — we do currently require signing a CLA for legal reasons. Should be possible via CLA Assistant

Screenshot 2022-09-16 at 10 49 34 AM I actually did sign a CLA for this but every time it shows me this screen saying that the CLA has been signed but it still says pending on the PR.

Imamah-Zafar avatar Sep 16 '22 08:09 Imamah-Zafar

Thanks, it might be possible that CLA can't connect the email to the GitHub user. Maybe try updating the commit email to the same as the GitHub user.

Sorry about these annoyances.

janniks avatar Sep 16 '22 10:09 janniks

@janniks for now we can use the screenshot as evidence of the signed CLA and merge this PR?

diwakergupta avatar Sep 16 '22 14:09 diwakergupta

Thanks 🙏🏻

janniks avatar Sep 16 '22 14:09 janniks