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

Wallet Decrypt Redesign #677

Merged
merged 31 commits into from
Jan 1, 2018
Merged

Wallet Decrypt Redesign #677

merged 31 commits into from
Jan 1, 2018

Conversation

wbobeirne
Copy link
Contributor

Closes #565 and addresses a few of #557's checkboxes. Rather than list all the changes I made, I think it's just best to watch it in action.

Screenshots / Gfycats

Desktop Account Unlock (Gfycat)

Mobile Account Unlock (Gfycat)

Sign & Verify

screen shot 2017-12-28 at 12 49 58 pm

Contract Deploy

screen shot 2017-12-28 at 12 56 30 pm

Contract Interact

screen shot 2017-12-28 at 12 56 52 pm

Caveats

  • Some people might not be into the animations. Let me know what you think of them.
  • Expanding to additional wallets (Keepkey?) will be a little more difficult to fit into this design than adding another radio button.
  • WalletDecrypt.tsx's code has become a little ugly for my tastes. Would love feedback on that.
  • I think there are a few outstanding PRs that touch unlock. I'll be sure to handle their conflicts, whoever's get merged first.
  • The wrapping it does in some at some of the intermediate sizes is a little janky. Ideally they'd wrap 2 by 2, but the markup required for that was pretty dirty.

screen shot 2017-12-28 at 1 02 54 pm

@wbobeirne wbobeirne added the status: ready for review Open PR's that are ready for technical review. Replaced with "changes needed" or "ready to merge". label Dec 28, 2017
)}
</div>
) : (
<WalletDecrypt hidden={unlocked} disabledWallets={DISABLE_WALLETS.UNABLE_TO_SIGN} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skubakdj this was my suggested change for how we use the disabledWallets prop. LMK if it looks good to you.

transform: translate(-50%, -100%) translateY(-8px);
transition-delay: 400ms, 400ms, 300ms;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt that this was the lowest friction way to implement tooltips, instead of making the component take in a whole bunch of props for showing and hiding. You simply add this mixin to the parent element you want to show the tooltip on hover. Let me know if y'all would rather have it be component props. Maybe we can do either / or?

@@ -82,7 +82,7 @@
"x_Json": "Αρχείο JSON (μη κρυπτογραφημένο) ",
"x_JsonDesc": "Αυτή είναι η μη κρυπτογραφημένη, JSON μορφή του ιδιωτικού κλειδιού σας. Αυτό σημαίνει ότι δεν απαιτείται συνθηματικό όμως οποιοσδήποτε βρει το JSON σας έχει πρόσβαση στο πορτοφόλι και στον αιθέρα σας χωρίς συνθηματικό. ",
"x_Keystore": "Αρχείο Keystore (UTC / JSON · Συνιστάται · Κρυπτογραφημένο) ",
"x_Keystore2": "Αρχείο Keystore (UTC / JSON) ",
"x_Keystore2": "Αρχείο Keystore ",
"x_KeystoreDesc": "Αυτό το αρχείο Keystore έχει την ίδια μορφή που χρησιμοποιείται από το Mist ώστε να μπορείτε εύκολα να το εισάγετε στο μέλλον. Είναι το συνιστώμενο αρχείο για λήψη και δημιουργία αντιγράφου ασφαλείας. ",
"x_MetaMask": "Metamask / Mist ",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Do the same Metamask / Mist split I did in en.json to all languages.

@dternyak
Copy link
Contributor

dternyak commented Dec 28, 2017

Still need to review implementation, but great work from a usability perspective 👍

A few issues I found on the UI / usability side:

  1. Inability to change wallets after decrypting:

    • sign message
    • deploy contract
  2. Padding(?) issues with helper text

screen shot 2017-12-28 at 12 25 16 pm

  1. Significant white space / relatively small input fields. In the case of view-only wallets, users will need to scroll horizontally to see the entire pasted address, but there is plenty of whitespace to expand into.

screen shot 2017-12-28 at 12 26 00 pm

(Opinionated) I think vertically and horizontally centering the unlock view would be an improvement.
screen shot 2017-12-28 at 12 27 13 pm

Will be back with a code review later today 😄

@@ -81,9 +81,11 @@
"x_Json": "JSON File (unencrypted) ",
"x_JsonDesc": "This is the unencrypted, JSON format of your private key. This means you do not need the password but anyone who finds your JSON can access your wallet & Ether without the password. ",
"x_Keystore": "Keystore File (UTC / JSON · Recommended · Encrypted) ",
"x_Keystore2": "Keystore File (UTC / JSON) ",
"x_Keystore2": "Keystore File ",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this change OK? Or should I make x_Keystore3?

Copy link
Contributor

@dternyak dternyak Dec 28, 2017

Choose a reason for hiding this comment

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

@tayvano Feel free to chime in with your thoughts.

My understanding is we'll do a once-over for all of the copy closer to release, and can address issues at that point. We'll need to merge updated translations from V3 and create keys for all raw markup in the codebase as well.

So no big deal either way.

@dternyak
Copy link
Contributor

dternyak commented Dec 29, 2017

While MetaMask is likely the most popular option, we may want to make users aware than any web3 provider is supported.

@wbobeirne
Copy link
Contributor Author

wbobeirne commented Dec 31, 2017

While MetaMask is likely the most popular option, we may want to make users aware than any web3 provider is supported.

I forgot to mention this up top, but it detects that metamask is installed and shows that. Also detects mist:

With no known web3 client detected

screen shot 2017-12-30 at 8 41 22 pm

With Mist

screen shot 2017-12-30 at 8 42 34 pm

@wbobeirne
Copy link
Contributor Author

@dternyak

  1. Added "Change Wallet" buttons on sign and deploy tabs.
  2. Removed outline on these buttons. They have hover and active states, so it should be fine without.
  3. Height is now auto instead of being as big for unlock, so the whitespaciness should feel better.

@dternyak dternyak merged commit 371e6e3 into develop Jan 1, 2018
@dternyak dternyak deleted the wallet-decrypt-redesign branch January 1, 2018 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready for review Open PR's that are ready for technical review. Replaced with "changes needed" or "ready to merge".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants