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

Alert user upon switching to unconnected account #8312

Merged
merged 3 commits into from
Apr 29, 2020
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Apr 9, 2020

An alert is now shown when the user switches from an account that is connected to the active tab to an account that is not connected. The alert prompts the user to dismiss the alert or connect the account they're switching to.

The "loading" state is handled by disabling the buttons, and the error state is handled by displaying a generic error message and disabling the connect button.

The new reducer for this alert has been created with createSlice from the Redux Toolkit. This utility is recommended by the Redux team, and represents a new style of writing reducers that I hope we will use more in the future (or at least something similar). createSlice constructs a reducer, actions, and action creators automatically. The reducer is constructed using their createReducer helper, which uses Immer to allow directly mutating the state in the reducer but exposing these changes as immutable.

@Gudahtt Gudahtt force-pushed the switch-account-alert branch from 29aa4d0 to 256e2b1 Compare April 16, 2020 04:57
@Gudahtt Gudahtt changed the base branch from develop to add-permitted-account April 16, 2020 04:57
Gudahtt added a commit that referenced this pull request Apr 16, 2020
This method adds the given account to the given origin's list of
exposed accounts. This method is not yet used, but it will be in
subsequent PRs (e.g. #8312)

This method has been added to the background API, and a wrapper action
creator has been written as well.
@Gudahtt Gudahtt force-pushed the add-permitted-account branch from 81a5e72 to 50c0166 Compare April 16, 2020 05:04
@Gudahtt Gudahtt force-pushed the switch-account-alert branch from 256e2b1 to fa1846d Compare April 16, 2020 05:04
@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 16, 2020

I'll wait on #8345 before fixing the broken integration tests, as I'll have one less state file to modify after that's merged.

@Gudahtt Gudahtt force-pushed the switch-account-alert branch from fa1846d to 8a90007 Compare April 16, 2020 15:49
@metamaskbot
Copy link
Collaborator

Builds ready [8a90007]
Page Load Metrics (650 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint348444115
domContentLoaded3457226497737
load3467246507737
domInteractive3447226487737

@Gudahtt Gudahtt force-pushed the add-permitted-account branch from 50c0166 to 86acf73 Compare April 16, 2020 16:46
@Gudahtt Gudahtt force-pushed the switch-account-alert branch from 8a90007 to 54d525c Compare April 16, 2020 16:47
@metamaskbot
Copy link
Collaborator

Builds ready [54d525c]
Page Load Metrics (669 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint34574163
domContentLoaded3458326689545
load3478356699546
domInteractive3458326679545

@Gudahtt Gudahtt force-pushed the add-permitted-account branch from 86acf73 to 0c969dc Compare April 16, 2020 18:20
@Gudahtt Gudahtt force-pushed the switch-account-alert branch from 54d525c to 918706e Compare April 16, 2020 18:21
@metamaskbot
Copy link
Collaborator

Builds ready [918706e]
Page Load Metrics (660 ± 54 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint35544363
domContentLoaded32980365811354
load33180566011254
domInteractive32980365811354

Gudahtt added a commit that referenced this pull request Apr 16, 2020
This method adds the given account to the given origin's list of
exposed accounts. This method is not yet used, but it will be in
subsequent PRs (e.g. #8312)

This method has been added to the background API, and a wrapper action
creator has been written as well.
@Gudahtt Gudahtt force-pushed the add-permitted-account branch from 0c969dc to 3970922 Compare April 16, 2020 18:58
@Gudahtt Gudahtt force-pushed the switch-account-alert branch from 918706e to 896ff3b Compare April 16, 2020 18:59
@metamaskbot
Copy link
Collaborator

Builds ready [896ff3b]
Page Load Metrics (695 ± 31 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint378646126
domContentLoaded6208586936531
load6228596956531
domInteractive6208576936531

Gudahtt added a commit that referenced this pull request Apr 16, 2020
This method adds the given account to the given origin's list of
exposed accounts. This method is not yet used, but it will be in
subsequent PRs (e.g. #8312)

This method has been added to the background API, and a wrapper action
creator has been written as well.
@Gudahtt Gudahtt force-pushed the switch-account-alert branch from 896ff3b to 22d9a13 Compare April 16, 2020 22:24
@Gudahtt Gudahtt changed the base branch from add-permitted-account to develop April 16, 2020 22:24
@metamaskbot
Copy link
Collaborator

Builds ready [22d9a13]
Page Load Metrics (685 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint379048115
domContentLoaded39681668311656
load39781868511756
domInteractive39681668311656

@Gudahtt Gudahtt force-pushed the switch-account-alert branch 2 times, most recently from 7b02acc to 08e71d0 Compare April 17, 2020 00:39
@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 17, 2020

Screenshots:

Here's the alert:

normal

Here is the error state:

error

@Gudahtt Gudahtt marked this pull request as ready for review April 17, 2020 00:45
@Gudahtt Gudahtt requested a review from a team as a code owner April 25, 2020 04:53
@Gudahtt Gudahtt force-pushed the switch-account-alert branch from f60f9a1 to 48af2bc Compare April 25, 2020 22:34
@metamaskbot
Copy link
Collaborator

Builds ready [48af2bc]

@Gudahtt Gudahtt force-pushed the switch-account-alert branch from 48af2bc to 928734a Compare April 27, 2020 21:56
@metamaskbot
Copy link
Collaborator

Builds ready [928734a]

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.

Very clean and crisp! A pleasure to review. Excited to see Immer being used, even if indirectly.

One in-line comment, and also some feedback about styling/UX:

image

Full Screenshot for Scale
  • The buttons appear disproportionately large to me, relative to the size of the popover. Could we make them smaller?
    • Related: Where are the designs?
  • In testing this, I quickly got very annoyed with this popup. Can we add a "Don't show this again" checkbox?

test/unit/ui/app/actions.spec.js Outdated Show resolved Hide resolved
@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 28, 2020

Yeah, the buttons do look a bit large. I think those sizes are being dictated by our default button styles? I'll take a look. We have another card for updating the button styles across the extension to be rounded, as shown in the Figma designs, but that wasn't going to be in v8. Maybe I can tweak them to be a bit smaller though.

The designs are here.

The "Don't show this again" checkbox was going to be in a follow-up PR; there's a separate card for that on Trello I believe.

@rekmarks
Copy link
Member

@Gudahtt word, sounds good! Just me when you've decided what to do with the button styles.

Oh one more thing: doing any e2e tests for this in a follow-up, or what's the plan for that?

@Gudahtt Gudahtt force-pushed the switch-account-alert branch 3 times, most recently from d9e0349 to 4495c2a Compare April 28, 2020 14:27
@metamaskbot
Copy link
Collaborator

Builds ready [4495c2a]
Page Load Metrics (672 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31473742
domContentLoaded3877836707737
load3887866727737
domInteractive3877836707737

rekmarks
rekmarks previously approved these changes Apr 28, 2020
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.

LGTM!

Buttons and checkbox can be fixed in one or more follow-up PRs.

@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 28, 2020

Two updates have been made:

  • Use SELECTED_ADDRESS_CHANGED state instead of cached address

    The unconnected-account reducer no longer caches the selected address when the alert was opened. Instead a new event was added for when the selected address changes (SELECTED_ADDRESS_CHANGED). Now that we know exactly when the selected address changes, we don't need to cache the original account for comparison anymore.

  • Update button styles to reduce size

    The buttons are now closer in size to the buttons in the designs. The button styles will be updated to match the designs more closely in a separate PR.

Screenshots of updated buttons:

updated-buttons

@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 28, 2020

@rekmarks I started trying to write an e2e test for this, but it was a bit challenging because we can't actually test the real popup with Selenium. It doesn't allow interacting with browser-things outside of the page (even the keyboard shortcut is intercepted). Even if it did allow it, there's no way to focus on the popup either.

In other tests we fake the popup UI by navigating to popup.html and making the window vagely popup-sized. The activeTab state still won't be set in that case though, and I rely upon that for this alert. We'll need to find a way to fake the activeTab state - then this should be testable.

@metamaskbot
Copy link
Collaborator

Builds ready [f83340c]
Page Load Metrics (666 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31503952
domContentLoaded3838616648441
load3858636668541
domInteractive3838606648441

@rekmarks rekmarks self-requested a review April 28, 2020 22:55
rekmarks
rekmarks previously approved these changes Apr 28, 2020
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.

LGTM!

Nice fix for the buttons and selected address state.

Gudahtt added 3 commits April 29, 2020 02:23
An alert is now shown when the user switches from an account that is
connected to the active tab to an account that is not connected. The
alert prompts the user to dismiss the alert or connect the account
they're switching to.

The "loading" state is handled by disabling the buttons, and the error
state is handled by displaying a generic error message and disabling
the connect button.

The new reducer for this alert has been created with `createSlice` from
the Redux Toolkit. This utility is recommended by the Redux team, and
represents a new style of writing reducers that I hope we will use more
in the future (or at least something similar). `createSlice` constructs
a reducer, actions, and action creators automatically. The reducer is
constructed using their `createReducer` helper, which uses Immer to
allow directly mutating the state in the reducer but exposing these
changes as immutable.
The `unconnected-account` reducer no longer caches the selected address
when the alert was opened. Instead a new event was added for when the
selected address changes (`SELECTED_ADDRESS_CHANGED`). Now that we know
exactly when the selected address changes, we don't need to cache the
original account for comparison anymore.
The buttons are now closer in size to the buttons in the designs. The
button styles will be updated to match the designs more closely in a
separate PR.
@metamaskbot
Copy link
Collaborator

Builds ready [0818023]
Page Load Metrics (649 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint308641157
domContentLoaded3188326479043
load3228346498943
domInteractive3188326479043

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.

Re-approve after rebase.

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.

This looks good to me! I didn't test it out locally but it looks like @rekmarks did.

@Gudahtt Gudahtt merged commit 53feb20 into develop Apr 29, 2020
@Gudahtt Gudahtt deleted the switch-account-alert branch April 29, 2020 17:10
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