Redesign approve screen #7271

danjm · 2019-10-10T16:18:05Z

Fixes #7165

This PR implements a redesign of the approve screen

Demo video here: https://streamable.com/5xnt7

metamaskbot · 2019-10-15T19:39:02Z

Builds ready [9ceec92]

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

Gudahtt · 2019-10-29T15:06:33Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

+      <div className="edit-approval-permission">
+        <div className="edit-approval-permission__header">
+          <div className="edit-approval-permission__title">
+            Edit Permission


This should probably be a localized message, along with the other text in this component

Gudahtt · 2019-10-29T15:07:44Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

+            Spend limit permission
+          </div>
+          <div className="edit-approval-permission__edit-section__description">
+            Allow Uniswap to withdraw and spend up to the following amount:


The "Uniswap" here should be replaced with the name for whichever site is requesting approval.
The same goes for the "Spend limit requested by Uniswap" message below

Gudahtt

I encountered this PropType warning upon opening the Approve screen:

Warning: Failed prop type: Invalid prop `decimals` of type `string` supplied to `ConfirmApprove`, expected `number`.
    in ConfirmApprove (created by TokenTrackerWrappedComponent)
    in TokenTrackerWrappedComponent (created by Connect(TokenTrackerWrappedComponent))
    in Connect(TokenTrackerWrappedComponent) (created by Route)
    in Route (created by withRouter(Connect(TokenTrackerWrappedComponent)))
    in withRouter(Connect(TokenTrackerWrappedComponent)) (created by Route)
    in Route (created by ConfirmTransaction)
    in Switch (created by ConfirmTransaction)
    in ConfirmTransaction (created by Connect(ConfirmTransaction))
    in Connect(ConfirmTransaction) (created by Route)
    in Route (created by withRouter(Connect(ConfirmTransaction)))
    in withRouter(Connect(ConfirmTransaction)) (created by Route)
    in Route (created by Authenticated)
    in Authenticated (created by Connect(Authenticated))
    in Connect(Authenticated) (created by Routes)
    in Switch (created by Routes)
    in div (created by Routes)
    in div (created by Routes)
    in Routes (created by Connect(Routes))
    in Connect(Routes) (created by Route)
    in Route (created by withRouter(Connect(Routes)))
    in withRouter(Connect(Routes)) (created by Index)
    in I18nProvider (created by Connect(I18nProvider))
    in Connect(I18nProvider) (created by Route)
    in Route (created by withRouter(Connect(I18nProvider)))
    in withRouter(Connect(I18nProvider)) (created by Index)
    in MetaMetricsProvider (created by Connect(MetaMetricsProvider))
    in Connect(MetaMetricsProvider) (created by Route)
    in Route (created by withRouter(Connect(MetaMetricsProvider)))
    in withRouter(Connect(MetaMetricsProvider)) (created by Index)
    in Router (created by HashRouter)
    in HashRouter (created by Index)
    in Provider (created by Index)
    in Index

I also noticed that the popup itself had a few UI issues:

Why is there a vertical scrollbar? It looks like a horizontal scrollbar appears upon opening the transaction details
The account is missing from the top-left
It seems like there should be more whitespace above and below the network (it looks a bit cramped). Maybe this would be fixed indirectly after adding the account though.

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

Gudahtt · 2019-10-30T18:02:53Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

+              })}>
+                {
+                  tokenAmount < tokenBalance
+                    ? -'Proposed Approval Limit'


is that - intended? I think this'd come out as NaN

Gudahtt · 2019-10-30T18:05:34Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

+          <div className="edit-approval-permission__edit-section__option">
+            <div
+              className="edit-approval-permission__edit-section__radio-button"
+              onClick={() => this.setState({ selectedOption: 'unlimited' })}


Nit: 'unlimited' and 'custom' should probably be enums. It seems like a good place to use an enum anyway.

I'll upgrade this from a nitpick to an issue: there are a lot of 'unlimited' and 'custom' scattered throughout this component, we should move those into constants at a minimum

We could also maybe even put this.state.selectedOption === 'custom' and this.state.selectedOption === 'unlimited' into state so that a lot of those instances could be read from state, e.g.

<div className={classnames({ 'edit-approval-permission__edit-section__radio-button-outline': !this.state.selectedOptionIsUnlimited, 'edit-approval-permission__edit-section__radio-button-outline--selected': this.state.selectedOptionIsUnlimited })} />

Gudahtt · 2019-10-30T18:12:13Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

+        submitType="primary"
+        contentClass="edit-approval-permission-modal-content"
+        containerClass="edit-approval-permission-modal-container"
+        submitDisabled={ this.state.customSpendLimit === this.props.customTokenAmount }


This prevents saving Unlimited if the custom amount selected is equal to the custom token amount.

e.g.

Set a custom spend limit of 5, then save it

Edit permissions again

Set a custom limit of 5

Select 'Unlimited'

Save is then disabled, when it should be enabled.

Actually, the onSubmit handler doesn't seem to allow setting 'Unlimited' anyway 🤔

This is indeed curious

Gudahtt · 2019-10-30T18:14:27Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.container.js

+
+const mapDispatchToProps = dispatch => {
+  return {
+    resetAccount: () => dispatch(resetAccount()),


I'm guessing you forgot to update this part

This still looks wrong - resetAcocunt isn't sued in the EditApprovalPermission component

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

whymarrh · 2019-10-31T19:23:41Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

+          <div className="edit-approval-permission__edit-section__option">
+            <div
+              className="edit-approval-permission__edit-section__radio-button"
+              onClick={() => this.setState({ selectedOption: 'unlimited' })}


I'll upgrade this from a nitpick to an issue: there are a lot of 'unlimited' and 'custom' scattered throughout this component, we should move those into constants at a minimum

whymarrh · 2019-10-31T19:26:38Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

+          <div className="edit-approval-permission__edit-section__option">
+            <div
+              className="edit-approval-permission__edit-section__radio-button"
+              onClick={() => this.setState({ selectedOption: 'unlimited' })}


We could also maybe even put this.state.selectedOption === 'custom' and this.state.selectedOption === 'unlimited' into state so that a lot of those instances could be read from state, e.g.

<div className={classnames({ 'edit-approval-permission__edit-section__radio-button-outline': !this.state.selectedOptionIsUnlimited, 'edit-approval-permission__edit-section__radio-button-outline--selected': this.state.selectedOptionIsUnlimited })} />

whymarrh · 2019-10-31T19:27:34Z

ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js

+        submitType="primary"
+        contentClass="edit-approval-permission-modal-content"
+        containerClass="edit-approval-permission-modal-container"
+        submitDisabled={ this.state.customSpendLimit === this.props.customTokenAmount }


This is indeed curious

whymarrh · 2019-10-31T19:28:56Z

ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js

+    return (
+      <div className="confirm-approve-content__transaction-details-content">
+        <div className="confirm-approve-content__small-text">
+          A fee is associated with this request. Learn why


The text in this component should also be localizable

…rmission

…sion

…type

…s the correct type

danjm · 2019-11-04T19:56:07Z

@Gudahtt @whymarrh all issues have been addressed:

792bacd Ensure first param passed to calcTokenValue in confirm-approve.util is the correct type
1dde6ff Ensure decimals prop passted to confirm-approve.component is correct type
bb5eeee Fix height of confirm-approval popup
f12010f Allow setting of approval amount to unlimited in edit-approval-permission
edceee2 Set option to custom on input change in edit-approval-permission
c886c68 Use state prop bool for unlimited vs custom check in edit-approval-permission
db0f9eb Show account in header of approve screen
3e0e22b Add translations to approve screen components

danjm · 2019-11-04T19:56:39Z

Video of editing permission amount: https://streamable.com/m6sdt

metamaskbot · 2019-11-05T00:40:15Z

Builds ready [1d35558]

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

ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js

metamaskbot · 2019-11-05T03:29:21Z

Builds ready [5de79be]

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

whymarrh

Seems legit

fixed

The units for the amounts shown on the approve screen in the transaction fee section were missing. It appears that they were present in an early version of the approve screen (#7271) but they got lost somewhere along the way.

danjm requested review from Gudahtt and whymarrh as code owners October 10, 2019 16:18

danjm force-pushed the new-approve-screens branch from f7a19e0 to 9ceec92 Compare October 15, 2019 19:19

Gudahtt requested changes Oct 29, 2019

View reviewed changes

Gudahtt previously requested changes Oct 30, 2019

View reviewed changes

whymarrh suggested changes Oct 31, 2019

View reviewed changes

danjm added 9 commits November 4, 2019 16:22

Redesign approve screen

e9f09b3

Add translations to approve screen components

3e0e22b

Show account in header of approve screen

db0f9eb

Use state prop bool for unlimited vs custom check in edit-approval-pe…

c886c68

…rmission

Set option to custom on input change in edit-approval-permission

edceee2

Allow setting of approval amount to unlimited in edit-approval-permis…

f12010f

…sion

Fix height of confirm-approval popup

bb5eeee

Ensure decimals prop passted to confirm-approve.component is correct …

1dde6ff

…type

Ensure first param passed to calcTokenValue in confirm-approve.util i…

792bacd

…s the correct type

danjm force-pushed the new-approve-screens branch from 9ceec92 to 792bacd Compare November 4, 2019 19:55

Fix e2e test of permission editing

1d35558

danjm force-pushed the new-approve-screens branch from 7988deb to 1d35558 Compare November 5, 2019 00:19

whymarrh suggested changes Nov 5, 2019

View reviewed changes

ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js Show resolved Hide resolved

ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js Show resolved Hide resolved

Remove unused code from edit-approval-permission.container

5de79be

danjm force-pushed the new-approve-screens branch from d4a8ec8 to 5de79be Compare November 5, 2019 03:10

whymarrh approved these changes Nov 5, 2019

View reviewed changes

danjm merged commit 2673eef into develop Nov 5, 2019

whymarrh deleted the new-approve-screens branch November 6, 2019 13:47

Gudahtt mentioned this pull request Nov 18, 2019

Show transaction fee units on approve screen #7468

Merged

bbondy mentioned this pull request Dec 16, 2019

Rebase Crypto Wallets to MetaMask upstream brave/brave-browser#7408

Closed

This was referenced Dec 15, 2020

UX to show "unlimited" on Approve screen per EIP717 #6469

Closed

Make "Approve" confirmation screen more informative MetaMask/Design#38

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign approve screen #7271

Redesign approve screen #7271

danjm commented Oct 10, 2019

metamaskbot commented Oct 15, 2019

Gudahtt Oct 29, 2019

Gudahtt Oct 29, 2019

Gudahtt left a comment

Gudahtt Oct 30, 2019

Gudahtt Oct 30, 2019

whymarrh Oct 31, 2019

whymarrh Oct 31, 2019

Gudahtt Oct 30, 2019

Gudahtt Oct 30, 2019

whymarrh Oct 31, 2019

Gudahtt Oct 30, 2019

Gudahtt Nov 4, 2019

whymarrh Oct 31, 2019

whymarrh Oct 31, 2019

whymarrh Oct 31, 2019

whymarrh Oct 31, 2019

danjm commented Nov 4, 2019

danjm commented Nov 4, 2019

metamaskbot commented Nov 5, 2019

metamaskbot commented Nov 5, 2019

whymarrh left a comment

Redesign approve screen #7271

Redesign approve screen #7271

Conversation

danjm commented Oct 10, 2019

metamaskbot commented Oct 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjm commented Nov 4, 2019

danjm commented Nov 4, 2019

metamaskbot commented Nov 5, 2019

metamaskbot commented Nov 5, 2019

whymarrh left a comment

Choose a reason for hiding this comment