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

Connections settings tab #7013

Merged
merged 5 commits into from
Aug 15, 2019
Merged

Conversation

whymarrh
Copy link
Contributor

@whymarrh whymarrh commented Aug 14, 2019

Refs #4703, #6904, #6955

This PR does a few things:

  • This PR removes the Share Address notification added in Privacy mode as default #6904
  • This PR removes the Privacy Mode toggle from settings
  • This PR introduces a new Connections tab in settings that allows users to add and review sites that have access to their accounts
  • This PR makes the list of connected sites persistent across MetaMask reloads

Some context:

  • As of Privacy mode as default #6904 (released in v7.0.0) Privacy Mode is on by default for all new users
  • Any existing users have/will be migrated to Privacy Mode
  • Privacy Mode means that sites must request account access à la EIP-1102 (or, as of this PR, they can be explicitly added)
Screenshots

Some finer details:

  • The input for adding a site doesn't currently enforce a valid domain
  • The input for adding a site IS pre-populated in the popup (via the browser action) with the active tab's origin

@whymarrh whymarrh force-pushed the connected-dapps-settings branch 2 times, most recently from 29a6143 to dd29ae6 Compare August 15, 2019 15:43
@danjm
Copy link
Contributor

danjm commented Aug 15, 2019

First question I have is whether we are certain we want to persist all connect request approvals? This will be a change in user experience. Might there be some users who like the default of these not being persisted?

@whymarrh
Copy link
Contributor Author

whymarrh commented Aug 15, 2019

  1. I feel like it is an improvement in the UX (that is, I think it is a good idea and I stand by it)
  2. I can drop dd29ae6 if we don't, no sweat

@Gudahtt
Copy link
Member

Gudahtt commented Aug 15, 2019

First question I have is whether we are certain we want to persist all connect request approvals? This will be a change in user experience. Might there be some users who like the default of these not being persisted?

Good point. While I'm sure most users would prefer them be persisted, there are sure to be those that prefer nothing to be saved between sessions.

The ideal solution might be a toggle, perhaps? One that defaults to persisting connections. I'm not sure how important that'd be to include in this release though. 🤔

@whymarrh
Copy link
Contributor Author

The ideal solution might be a toggle, perhaps? One that defaults to persisting connections. I'm not sure how important that'd be to include in this release though. 🤔

I don't think that's quite ideal. I think we can comfortably pick one way and stick with it.

@whymarrh whymarrh force-pushed the connected-dapps-settings branch from dd29ae6 to 5bbda44 Compare August 15, 2019 16:27
@danjm
Copy link
Contributor

danjm commented Aug 15, 2019

First question I have is whether we are certain we want to persist all connect request approvals? This will be a change in user experience. Might there be some users who like the default of these not being persisted?

I guess the remove all button is there for users who really care

danjm
danjm previously approved these changes Aug 15, 2019
@Gudahtt
Copy link
Member

Gudahtt commented Aug 15, 2019

I don't think that's quite ideal. I think we can comfortably pick one way and stick with it.

I was thinking of it like an "Incognito mode". Certainly nothing we need right now though, granted.

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! I noticed a few stray props that should be removed, but everything's good aside from that.

app/scripts/metamask-controller.js Show resolved Hide resolved
ui/app/pages/home/home.component.js Show resolved Hide resolved
app/_locales/ht/messages.json Show resolved Hide resolved
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.

5 participants