Consolidate connected account alerts #8802

rekmarks · 2020-06-12T21:17:48Z

Screenshot

Consolidates the two former connected account alerts into one, and updates it to use the ConnectedAccountsList component.

Summary

Consolidates switch-to-connected alert into unconnected-account-alert
- Use ConnectedAccountsList to implement new designs
- Handle both connecting the currently selected account and switching to connected accounts
Extract ConnectedAccountPermissions from ConnectedAccountsList folder
- This is now used in the ConnectedAccounts component instead of ConnectedAccountsList, in the Popover footer
Various updates to ConnectedAccountsList and other components to support these changes
- ConnectedAccountsList is functionally identical to develop

Open Questions

Currently, the alert can open on e.g. permissions and transactions confirmations in the popup. Is that what we want?

ui/app/components/app/alerts/unconnected-account-alert/unconnected-account-alert.scss

rekmarks · 2020-06-13T20:30:51Z

ui/app/components/app/connected-accounts-list/connected-accounts-list.component.js

+    shouldRenderListOptions: (props, propName, componentName) => {
+      if (props[propName]) {
+        if (typeof props[propName] !== 'boolean') {
+          return new Error(
+            `${componentName}: '${propName}' must be a boolean if provided.`
+          )
+        }
+        if (!props['removePermittedAccount']) {
+          return new Error(
+            `${componentName}: '${propName}' requires 'removePermittedAccount'.`
+          )
+        }
+      }
+    },


removePermittedAccount is used in disconnectAccount, which is only called if the list item options are rendered. So, if shouldRenderListOptions === true, removePermittedAccount must be provided. Otherwise, we don't care.

I'm not completely happy with this solution, but it works. I'm very open to suggestions!

ui/app/pages/connected-accounts/connected-accounts.component.js

metamaskbot · 2020-06-13T21:29:43Z

Builds ready [6db7890]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (1189 ± 68 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	933	1454	1186	141	68
		load	942	1460	1189	141	68
		domInteractive	933	1454	1185	141	68

metamaskbot · 2020-06-15T05:36:21Z

Builds ready [2532b89]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (651 ± 24 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	85	45	19	9
		domContentLoaded	608	839	649	51	25
		load	610	840	651	51	24
		domInteractive	608	838	649	51	25

ui/app/pages/connected-accounts/connected-accounts.container.js

Gudahtt · 2020-06-15T14:22:19Z

ui/app/helpers/utils/util.js

+/**
+ * Checks whether a URL-like value (object or string) is an extension URL.
+ *
+ * @param {string | URL | object} urlLike - The URL-like value to test.


Nit: Why make this function more complicated than it needs to be? We only ever pass in a string.

activeTab is not a string, but an object with a protocol property, much like a URL object.

Of course, knowing that, we could simply pass it the protocol string of that object. I'm fine with that if you prefer.

Or the origin, which is what we used to be comparing against.

It doesn't really matter. Just that it's nice to avoid branches if possible, and it's nice to have clearer type signatures.

ui/app/components/app/connected-accounts-list/connected-accounts-list.component.js

ui/app/components/app/alerts/unconnected-account-alert/unconnected-account-alert.scss

ui/app/components/app/connected-accounts-list/index.scss

ui/app/pages/connected-accounts/index.scss

metamaskbot · 2020-06-15T16:46:31Z

Builds ready [c78e377]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (644 ± 11 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	76	36	10	5
		domContentLoaded	597	679	643	23	11
		load	599	680	644	23	11
		domInteractive	597	678	642	23	11

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

rekmarks · 2020-06-15T17:05:30Z

:only-child selectors fixed in: bb9bb6a

metamaskbot · 2020-06-15T17:27:53Z

Builds ready [bb9bb6a]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (871 ± 57 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	39	114	72	26	12
		domContentLoaded	487	1182	869	118	57
		load	489	1185	871	118	57
		domInteractive	487	1182	869	118	57

metamaskbot · 2020-06-15T18:55:31Z

Builds ready [ef4aabe]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (694 ± 39 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	82	39	11	5
		domContentLoaded	454	864	692	81	39
		load	456	866	694	81	39
		domInteractive	453	864	692	81	39

Gudahtt

LGTM!

* origin/develop: (58 commits) Fix site icon fallback letter (#8815) add hover style to list-item (#8813) Fix site icon size (#8814) Consolidate connected account alerts (#8802) lowercase web3 Use markdown-to-jsx@6.11.4 (#8809) Update app/_locales/en/messages.json Update app/_locales/en/messages.json also remove 'dapp' from descriptions remove all user-facing instances of 'dapp' update button styling on home/asset page (#8800) Fix handling of permissions of removed accounts (#8803) Clear permssions during createNewVaultAndRestore (#8804) Hide token transfers on ETH asset page (#8799) Fix account name editing (#8801) Fix connect flow account list height (#8798) Update color of menu item icons (#8797) Update "Connected accounts" empty description (#8796) Stop reporting failed transactions to Sentry (#8795) Omit state snapshot from Sentry errors (#8794) ...

rekmarks added the DO-NOT-MERGE Pull requests that should not be merged label Jun 12, 2020

rekmarks force-pushed the consolidate-connected-account-alerts branch 3 times, most recently from b3257a1 to c1c561c Compare June 13, 2020 20:21

rekmarks commented Jun 13, 2020

View reviewed changes

ui/app/components/app/alerts/unconnected-account-alert/unconnected-account-alert.scss Outdated Show resolved Hide resolved

rekmarks commented Jun 13, 2020

View reviewed changes

ui/app/pages/connected-accounts/connected-accounts.component.js Show resolved Hide resolved

rekmarks force-pushed the consolidate-connected-account-alerts branch from ddb5a21 to 6db7890 Compare June 13, 2020 20:53

rekmarks removed the DO-NOT-MERGE Pull requests that should not be merged label Jun 13, 2020

rekmarks marked this pull request as ready for review June 13, 2020 21:03

rekmarks requested review from kumavis and a team as code owners June 13, 2020 21:03

rekmarks added 9 commits June 14, 2020 21:17

update connected accounts appearance

97f3732

consolidate account alerts

12ec946

UnconnectedAccountAlert: use ConnectedAccountsList

ac01e72

fixup! UnconnectedAccountAlert: use ConnectedAccountsList

f4da229

fix alert error display; cleanup

c3b282e

fixup! fix alert error display; cleanup

9abe6de

fix isExtensionUrl helper function

a4a324a

fix connectAccount in alert

1859f17

fixup! fix connectAccount in alert

2532b89

rekmarks mentioned this pull request Jun 15, 2020

if i switch to a site that isn't connected and click "don't show this again" i can't click connect #8732

Closed

rekmarks force-pushed the consolidate-connected-account-alerts branch from 6db7890 to 2532b89 Compare June 15, 2020 04:37

Gudahtt reviewed Jun 15, 2020

View reviewed changes

ui/app/components/app/connected-accounts-list/connected-accounts-list.component.js Outdated Show resolved Hide resolved

rekmarks added 2 commits June 15, 2020 08:51

pass address to connectAccount

d42ced2

fix accounts list custom prop validator

c78e377

Gudahtt reviewed Jun 15, 2020

View reviewed changes

rekmarks and others added 3 commits June 15, 2020 09:50

Update ui/app/components/app/connected-accounts-list/index.scss

ea434da

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

css fixups

a01cd50

fix only-child selectors

bb9bb6a

move switch account action out of menu in all views

ef4aabe

brad-decker approved these changes Jun 15, 2020

View reviewed changes

Gudahtt approved these changes Jun 15, 2020

View reviewed changes

rekmarks merged commit 4dfe4e7 into develop Jun 15, 2020

rekmarks deleted the consolidate-connected-account-alerts branch June 15, 2020 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate connected account alerts #8802

Consolidate connected account alerts #8802

rekmarks commented Jun 12, 2020 •

edited

Loading

rekmarks Jun 13, 2020

metamaskbot commented Jun 13, 2020

metamaskbot commented Jun 15, 2020

Gudahtt Jun 15, 2020

rekmarks Jun 15, 2020

rekmarks Jun 15, 2020 •

edited

Loading

Gudahtt Jun 15, 2020

metamaskbot commented Jun 15, 2020

rekmarks commented Jun 15, 2020

metamaskbot commented Jun 15, 2020

metamaskbot commented Jun 15, 2020

Gudahtt left a comment

Consolidate connected account alerts #8802

Consolidate connected account alerts #8802

Conversation

rekmarks commented Jun 12, 2020 • edited Loading

Summary

Open Questions

rekmarks Jun 13, 2020

Choose a reason for hiding this comment

metamaskbot commented Jun 13, 2020

metamaskbot commented Jun 15, 2020

Gudahtt Jun 15, 2020

Choose a reason for hiding this comment

rekmarks Jun 15, 2020

Choose a reason for hiding this comment

rekmarks Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Gudahtt Jun 15, 2020

Choose a reason for hiding this comment

metamaskbot commented Jun 15, 2020

rekmarks commented Jun 15, 2020

metamaskbot commented Jun 15, 2020

metamaskbot commented Jun 15, 2020

Gudahtt left a comment

Choose a reason for hiding this comment

rekmarks commented Jun 12, 2020 •

edited

Loading

rekmarks Jun 15, 2020 •

edited

Loading