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.

Sign up for GitHub

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

Jump to bottom

Add switch to connected account alert #8532

Merged
merged 8 commits into from
May 12, 2020
Merged

Add switch to connected account alert #8532

merged 8 commits into from
May 12, 2020

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 6, 2020

Add alert suggesting that the user switch to a connected account. This alert is displayed when the popup is opened over an active tab that is connected to some account, but not the current selected account. The user can choose to switch to a connected account, or dismiss the alert.

This alert is only shown once per account switch. So if the user repeatedly opens the popup on a dapp without switching accounts, it'll only be shown the first time.

A checkbox allowing the user to dismiss the alert permanently will be added in a subsequent PR.

@Gudahtt
Copy link
Member Author

Gudahtt commented May 6, 2020 •
edited
Loading

This depends upon #8524 This is now ready to review

@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch from bd14414 to ad5ba5c Compare May 6, 2020 16:12
@Gudahtt Gudahtt changed the base branch from add-dropdown-component to develop May 6, 2020 16:13
@metamaskbot
Copy link
Collaborator

Builds ready [ad5ba5c]
Page Load Metrics (658 ± 25 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298638126
domContentLoaded5867836565325
load5877856585325
domInteractive5867826565325

@Gudahtt Gudahtt marked this pull request as ready for review May 6, 2020 17:11
@Gudahtt Gudahtt requested review from kumavis and a team as code owners May 6, 2020 17:11
@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch 2 times, most recently from 353e19d to 5305132 Compare May 6, 2020 23:29
@Gudahtt Gudahtt marked this pull request as draft May 6, 2020 23:32
@Gudahtt
Copy link
Member Author

Gudahtt commented May 6, 2020 •
edited
Loading

I need to check on a couple of more things before this is ready to review:

  • The check for whether the alert was shown already isn't per-dapp right now, but it should be.
  • There's a scrollbar that keeps showing up on the home screen that I want to get rid of.

Edit: Both of these have been addressed. This PR is now ready for review again.

@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch from 5305132 to 096654c Compare May 6, 2020 23:40
@metamaskbot
Copy link
Collaborator

Builds ready [096654c]
Page Load Metrics (680 ± 17 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32443832
domContentLoaded6207766783617
load6217776803617
domInteractive6197766783617

@Gudahtt
Copy link
Member Author

Gudahtt commented May 7, 2020

Screenshots: Disregard the scrollbar in these two screenshots - that was removed in a separate PR (#8547)

Multiple accounts:

switch-to-alert

One account:

switch-to-alert-1-account

@Gudahtt Gudahtt marked this pull request as ready for review May 7, 2020 13:39
@metamaskbot
Copy link
Collaborator

Builds ready [151c2de]
Page Load Metrics (689 ± 26 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29433542
domContentLoaded6098276875326
load6108286895426
domInteractive6098266875326

@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch from 151c2de to 6b320b7 Compare May 8, 2020 01:18
@Gudahtt Gudahtt requested a review from rachelcope May 8, 2020 01:19
@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch from 6b320b7 to e5ef83a Compare May 8, 2020 01:29
@metamaskbot
Copy link
Collaborator

Builds ready [e5ef83a]
Page Load Metrics (605 ± 10 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint28403332
domContentLoaded5766546042010
load5776566052010
domInteractive5766546042010

@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch from e5ef83a to 72d6500 Compare May 8, 2020 02:37
@metamaskbot
Copy link
Collaborator

Builds ready [72d6500]
Page Load Metrics (650 ± 15 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3092462010
domContentLoaded6077286493215
load6087296503215
domInteractive6067276483215

rachelcope
rachelcope previously approved these changes May 8, 2020
View reviewed changes
Copy link

@rachelcope rachelcope left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One small design feedback - looks like there is too much left/right padding on the input (making it inconsistently aligned with the title and buttons).
Otherwise, LGTM!

@rachelcope
Copy link

Should this have the "don't show this again" functionality?

@Gudahtt
Copy link
Member Author

Gudahtt commented May 8, 2020

Should this have the "don't show this again" functionality?

Nope! Though I'm considering updating this PR to include it before merging, now that #8550 is pretty close to being merged.

@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch from 72d6500 to f581cb3 Compare May 8, 2020 18:24
@Gudahtt
Copy link
Member Author

Gudahtt commented May 8, 2020

looks like there is too much left/right padding on the input (making it inconsistently aligned with the title and buttons).

Good catch! I've just updating the input padding to match the text.

Here's what it looks like now:

updated-padding

@Gudahtt
Copy link
Member Author

Gudahtt commented May 8, 2020

I've also just realized that I handled the single-account case wrong. It's supposed to look like this if there's one account:

Screenshot:

switch-accounts

@metamaskbot
Copy link
Collaborator

Builds ready [f581cb3]
Page Load Metrics (635 ± 14 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298037115
domContentLoaded5896826332914
load5906846352914
domInteractive5896826332914

@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch from f581cb3 to 2c019b9 Compare May 8, 2020 19:49
@Gudahtt Gudahtt marked this pull request as draft May 8, 2020 20:09
@metamaskbot
Copy link
Collaborator

Builds ready [624dfa8]
Page Load Metrics (617 ± 12 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29503352
domContentLoaded5786716152512
load5806726172512
domInteractive5786716152512

@Gudahtt
Copy link
Member Author

Gudahtt commented May 8, 2020

The "don't show this again" checkbox has been added to this alert, along with the associated alert settings.

Screenshots:

Alert with checkbox:

with-checkbox

Updated alert settings:

alert-settings

@metamaskbot
Copy link
Collaborator

Builds ready [0a16313]
Page Load Metrics (742 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31107512211
domContentLoaded46092774110751
load46292974210752
domInteractive45892774010752

@Gudahtt Gudahtt marked this pull request as ready for review May 8, 2020 20:55
@Gudahtt
Copy link
Member Author

Gudahtt commented May 9, 2020

I've also just realized that I handled the single-account case wrong.

After discussing this with @rachelcope , the copy for both the single and multiple-account cases has been updated. The alert has also been updated to omit the dropdown if there is just a single account.

Here are the updated screenshots:

Single account:

new-one-account

Multiple accounts:

new-multiple-accounts

@metamaskbot
Copy link
Collaborator

Builds ready [bff2cac]
Page Load Metrics (724 ± 25 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint338545147
domContentLoaded6348027225125
load6368047245125
domInteractive6348027225125

Gudahtt added 8 commits May 8, 2020 21:50
@Gudahtt
Add switch to connected account alert
674164a 
Add alert suggesting that the user switch to a connected account. This
alert is displayed when the popup is opened over an active tab that is
connected to some account, but not the current selected account. The
user can choose to switch to a connected account, or dismiss the alert.

This alert is only shown once per account switch. So if the user
repeatedly opens the popup on a dapp without switching accounts, it'll
only be shown the first time.

A checkbox allowing the user to dismiss the alert permanently will be
added in a subsequent PR.
@Gudahtt
Track shown state per-domain
12ffc1a
@Gudahtt
Set alert as shown when showing similar alert
a69c301 
The "Switch to connceted" alert is now set as "shown" after the
"unconnected account" alert has been shown. This is to prevent the user
from being shown two very similar alerts in a short span of time.
@Gudahtt
Update padding around account selection input
020c404
@Gudahtt
Switch to using new ALERT_TYPES switchToConnected enum
14bbe46
@Gudahtt
Move 'switchToConnectedAlertShown' from app state controller to alert…
eb84815 
… controller
@Gudahtt
Add 'don't show this again' checkbox to alert
cd9baae
@Gudahtt
Update copy, update single-account design
ad5ee94 
The alert has been updated to omit the dropdown when there is just a
single connected account. The copy for both the single and multiple
account states has also been updated.
@Gudahtt Gudahtt force-pushed the add-switch-to-connected-alert branch from bff2cac to ad5ee94 Compare May 9, 2020 00:50
@metamaskbot
Copy link
Collaborator

Builds ready [ad5ee94]
Page Load Metrics (580 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint297837126
domContentLoaded32965157910350
load33165358010350
domInteractive32965157810350

whymarrh
whymarrh approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt Gudahtt merged commit 53ec42d into develop May 12, 2020
@Gudahtt Gudahtt deleted the add-switch-to-connected-alert branch May 12, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whymarrh whymarrh whymarrh approved these changes

@rachelcope rachelcope rachelcope left review comments

@kumavis kumavis Awaiting requested review from kumavis kumavis is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Gudahtt @metamaskbot @rachelcope @whymarrh