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

Add Connected Accounts modal #8313

Merged
merged 1 commit into from
May 15, 2020

Conversation

whymarrh
Copy link
Contributor

@whymarrh whymarrh commented Apr 9, 2020

This PR implements the Connected Accounts modal designs.

Screenshots

Screenshots of the unconnected flow

@whymarrh whymarrh force-pushed the connected-accounts-modal branch 2 times, most recently from f78f357 to 361ecd0 Compare April 13, 2020 01:54
@whymarrh whymarrh force-pushed the connected-accounts-modal branch 2 times, most recently from 8250515 to fd3009d Compare April 27, 2020 13:13
@whymarrh whymarrh force-pushed the connected-accounts-modal branch 6 times, most recently from e42c69e to 0d35cfb Compare May 5, 2020 19:29
@whymarrh whymarrh marked this pull request as ready for review May 5, 2020 19:52
@whymarrh whymarrh requested a review from a team as a code owner May 5, 2020 19:52
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Looks great!

In addition to the inline suggestion, the way we identify the Primary account label is not correct. The permitted accounts are no longer ordered in the permissions state. They have to be sorted by preferences.identities[account].lastSelected to identify which one is first. That's what we do before sending them to the dapp.

Currently, the Primary label is identified by whatever account is first in the permissions state.

Edit: We may also want to consider the name Primary. I was talking to @jennypollack about this earlier, and we want to encourage dapps to pay no attention to the order of the accounts returned from eth_accounts or accountsChanged, and let the dapp handle what the "primary" account is, if any. On the other hand, I'm not sure what that name should be. "Maybe the only account that the dapp will acknowledge until to switch to it in the MetaMask popup UI while browsing the dapp" doesn't exactly roll off the tongue.

ui/app/selectors/permissions.js Outdated Show resolved Hide resolved
@Gudahtt
Copy link
Member

Gudahtt commented May 6, 2020

we want to encourage dapps to pay no attention to the order of the accounts returned

I'm not sure we do want to do this, and I don't think it's something we can reasonably accomplish. A lot of dapps just want to deal with a single account, not a list, and I don't see why we'd object to this.

@whymarrh whymarrh force-pushed the connected-accounts-modal branch 3 times, most recently from 1ca63ad to 0ec38c6 Compare May 14, 2020 11:36
@whymarrh
Copy link
Contributor Author

I'm just addressing the portal comment now, everything else should be good

brad-decker
brad-decker previously approved these changes May 14, 2020
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM, just a question for learnin' reasons.

@whymarrh
Copy link
Contributor Author

@Gudahtt I moved the options into a tooltip in 76b80cc

brad-decker
brad-decker previously approved these changes May 14, 2020
@rekmarks rekmarks dismissed their stale review May 14, 2020 18:54

Changes made!

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks good so far! I haven't reviewed the styles in detail yet, but I'll do that shortly. The account sort order is the only blocker.

app/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/app/selectors/permissions.js Outdated Show resolved Hide resolved
ui/app/selectors/permissions.js Outdated Show resolved Hide resolved
@whymarrh whymarrh force-pushed the connected-accounts-modal branch 2 times, most recently from d99ee86 to cd301f0 Compare May 15, 2020 16:57
@whymarrh
Copy link
Contributor Author

There's a single outstanding (small) issue here where switching accounts via the options list will keep the tooltip open. I'm not sure if there's an option to configure both hiding the tooltip when clicked and to have it be interactive.

@whymarrh whymarrh force-pushed the connected-accounts-modal branch from cd301f0 to f050fe3 Compare May 15, 2020 17:29
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Two outstanding issues remain:

  • The account options menu is not great. It's weirdly slow to close (feels like almost a second!?), and doesn't close in all cases we'd want it to.
  • The sort order of the accounts still isn't quite correct - the secondary order is now dictated by the order of permissions (which isn't supposed to be meaningful) rather the keyring controller order. This would be just a minor nuisance/inconsistency except that it is possible for accounts to have never been selected, so the primary account may still be wrong in some cases.

The first is not a deal-breaker, and the second is a weird edge case, so I think this is fine to merge for now so that design QA can start. I'll address both of these issues in separate PRs.

@Gudahtt Gudahtt merged commit d488c16 into MetaMask:develop May 15, 2020
@whymarrh whymarrh deleted the connected-accounts-modal branch May 15, 2020 19:34
@Gudahtt
Copy link
Member

Gudahtt commented May 18, 2020

The account options menu is not great. It's weirdly slow to close (feels like almost a second!?), and doesn't close in all cases we'd want it to.

This has been addressed by #8607

The sort order of the accounts still isn't quite correct - the secondary order is now dictated by the order of permissions (which isn't supposed to be meaningful) rather the keyring controller order. This would be just a minor nuisance/inconsistency except that it is possible for accounts to have never been selected, so the primary account may still be wrong in some cases.

This has been addressed by #8608

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.

4 participants