Add token selection to the send screen #6445

chikeichan · 2019-04-11T09:02:36Z

A couple UX adjustments i've made @cjeria @bdresser

Title of the screen will either be "Send Ether" or "Send Tokens", depending on the asset selection
Changing asset actually changes token selected on main screen as well.

whymarrh · 2019-04-12T00:23:35Z

@chikeichan can we get this rebased on the latest develop?

chikeichan · 2019-04-12T03:23:09Z

ui/app/pages/send/send-content/send-asset-row/send-asset-row.component.js

+    this.setState({
+      isShowingDropdown: false,
+    }, () => {
+      this.context.metricsEvent({


@danjm Added a metric event - can you verify that I have used the right category and things?

Note: I am tracking whether or not the user selected a Token or Ether, instead of tracking the exact token address. Erring on the side of privacy.

@chikeichan this is correct

In addition, when we first add a custom variable, we need to map it to a specific id in metamask-extension/ui/app/helpers/utils/metametrics.util.js

However there are some limitations on how we do that and before we add anymore custom variables I want to reorganize the code and provide clear inline documentation.

Your code can stay in this PR. But let's not do the additional changes to the metrics util in this PR. I will do them as part of #6453 which I've added to next sprint. I have made a not that references your comment/code.

danjm

Looks good overall. Left a few comments. The one about selecting between multiple tokens while editing is the only really important one.

danjm · 2019-04-12T14:29:02Z

ui/app/ducks/metamask/metamask.js

        selectedTokenAddress: action.value,
      })
+      const newSend = extend(metamaskState.send)
+
+      if (metamaskState.send.editingTransactionId && !action.value) {


This is meant to handle the specific case of when we are editing a transaction and ETH is selected, correct?

I see in send-asset-row.component that we pass an empty string when ETH is selected. Perhaps instead of just !action.value, we could explicitly check action.value === ''. That way the reader of the code here knows that the value passed to the action was intentionally an empty string, and not just defaulting to undefined.

changed it to this.setSelectedToken() to invoke with no params, keeping it consistent with how it is call everywhere else in the app.

danjm · 2019-04-12T14:30:11Z

ui/app/ducks/metamask/metamask.js

        selectedTokenAddress: action.value,
      })
+      const newSend = extend(metamaskState.send)
+
+      if (metamaskState.send.editingTransactionId && !action.value) {


What about the case when a user has ETH + two or more tokens in the dropdown. Do we need to modify unapprovedTxs in state if, while editing a token transaction, a user switches it to a different token?

This part is handled in ui/app/pages/send/send.component.js:L169. Would love a manual test from you to see if it works. I tested it with multiple tokens locally many times.

danjm · 2019-04-12T14:32:50Z

ui/app/pages/send/send.component.js

+    const selectedTokenAddress = selectedToken && selectedToken.address
+
+    if (selectedTokenAddress && prevTokenAddress !== selectedTokenAddress) {
+      updateSendTokenBalance({


Can we use this.updateSendToken() here?

Nice catch! Updated

danjm · 2019-04-12T14:36:49Z

ui/app/pages/send/send.component.js

@@ -209,6 +225,7 @@ export default class SendTransactionScreen extends PersistentForm {
          updateGas={(updateData) => this.updateGas(updateData)}
          scanQrCode={_ => this.props.scanQrCode()}
          showHexData={showHexData}
+          editingTransactionId={editingTransactionId}


I don't think we need to pass editingTransactionId here

nice catch!

danjm · 2019-04-12T14:37:18Z

ui/app/pages/send/send-content/send-content.component.js


 export default class SendContent extends Component {

  static propTypes = {
    updateGas: PropTypes.func,
    scanQrCode: PropTypes.func,
    showHexData: PropTypes.bool,
+    editingTransactionId: PropTypes.string,


I don't think we need to include editingTransactionId in prop types here

cjeria · 2019-04-15T22:22:44Z

@chikeichan I notice that the spacing between each form element is inconsistent. As per the design system, we have spacing variables defined starting with 0, 4px, 8px, 16px, 24px. and so on. I'm aware that we'll want this to display nicely in the extension viewport as well, so I'd suggest we go with an 8px spacing between form elements (form rows). Adding token selection will push down the transaction fee form element (not ideal) and will end up looking like this, but I think reducing the spacing (to 8px) will help here.

cjeria · 2019-04-15T22:28:37Z

Two other thoughts:

We might want to consider opening a full screen tab when sending to remedy the lack of realestate in the extension viewport.
When we update the send flow with the address book, we'll get more realestate for the asset row.

chikeichan · 2019-04-16T06:49:28Z

Going to address @cjeria comment regarding padding tomorrow.

For the comment on changing send screen, I think that's too big of a UX change, and it would affect send flow initiated from a Dapp.

danjm · 2019-04-16T14:14:04Z

@chikeichan I believe the suggestion about the send screen is to just open the extension in a browser tab when the user goes to the send screen. This is done, for example, in the onImport method of metamask-extension/ui/app/pages/unlock-page/unlock-page.container.js

chikeichan · 2019-04-17T05:13:24Z

hmm.... I'm still not a big fan of introducing this change to the users. I think opening a new tab is a drastic UX change that we don't know if it's good or bad. Also, if it changes again in the near future when Address Book is shipped, we would be resetting users expectation when it comes to "what happen when I click on send" too many times within a short period of time.

A few suggestions, with a couple wild ones:

Perhaps we can adjust the size of the "Send" header? Now that there is an asset dropdown, the header text Send Ether/Send Token might not need to be as prominent.
Perhaps we can even put the Asset Dropdown INSIDE the header?
Or maybe we can use a modal for now, and change it to a dropdown when Address Book is launched.

@cjeria - what do you think 😃 ?

bdresser · 2019-04-17T17:14:23Z

can we reduce the top & bottom padding of the Transaction Fee buttons? They're unnecessarily tall and I've always wondered why

from this (height 74px i think)

to this (height 56 px)

danjm · 2019-04-17T17:27:20Z

@cjeria @bdresser Adjusted the spacing between send form rows and reduced the height of the gas buttons

cjeria · 2019-04-17T17:28:29Z

Nice catch @bdresser! Approved!

bdresser · 2019-04-17T18:15:29Z

wait @danjm

~~where'd the top bar go? i think it's important to let people switch networks on this screen, I do it all the time~~ nvm i see this is a screenshot from the full page view
looks like your ETH exchange rate isn't showing up -- make sure you leave extra vertical height in the gas buttons because they include a fiat conversion too

danjm · 2019-04-17T18:33:15Z

fixed @bdresser

danjm

Let's hold off on merging... I am seeing something weird while QAing on testnet

chikeichan requested review from danjm and whymarrh as code owners April 11, 2019 09:02

chikeichan added the needs review label Apr 11, 2019

chikeichan force-pushed the token-select branch from 0a8556a to 1c129c9 Compare April 12, 2019 02:33

chikeichan commented Apr 12, 2019

View reviewed changes

danjm mentioned this pull request Apr 12, 2019

Document metrics events and organize custom variables by page #6453

Closed

danjm requested changes Apr 12, 2019

View reviewed changes

omnat mentioned this pull request Apr 15, 2019

Add token selection to Send screen MetaMask/Design#72

Closed

chikeichan added 16 commits April 15, 2019 23:09

Move send to pages/

6b83031

Fix unit tests

9261d85

Finish UI

d750c5e

Integrate asset dropdown to send actions

f8459b7

Remove console.log

5714cc3

Hide asset change during edit

c9b18d1

Enable switch from send token to seand eth

4137e53

Enable switching from token to eth when editing

9798892

Fix linter

02339b1

Fixing test

74f0733

Fix unit tests

8d64e17

Fix linter

6fe32f3

Fix react warning; remove console.log

8e018e6

fix flat test

c51afaf

Add metrics

7595013

Address code review comments

a8efcd8

chikeichan force-pushed the token-select branch from 3eb299d to a8efcd8 Compare April 16, 2019 06:36

chikeichan requested a review from danjm April 16, 2019 06:46

danjm added 2 commits April 17, 2019 14:50

Consistent spacing between send screen form rows.

d8f1432

Reduce height of gas buttons on send screen.

2ac0302

danjm previously approved these changes Apr 17, 2019

View reviewed changes

Make send screen gas button height dependent on size of contents.

d6b9026

danjm dismissed their stale review via d6b9026 April 17, 2019 18:32

danjm approved these changes Apr 17, 2019

View reviewed changes

danjm requested changes Apr 17, 2019

View reviewed changes

danjm approved these changes Apr 17, 2019

View reviewed changes

danjm merged commit 931aaeb into MetaMask:develop Apr 17, 2019

danjm mentioned this pull request Apr 17, 2019

Add token selection to the send screen #4903

Closed

bdresser mentioned this pull request May 7, 2019

When sending tokens to smart contract address MetaMask blocks the transaction #6574

Closed

danjm mentioned this pull request May 17, 2019

Metametrics documentation #6624

Merged

bschorchit mentioned this pull request Feb 21, 2022

New UI: Send Token Screen Improvements #4772

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add token selection to the send screen #6445

Add token selection to the send screen #6445

chikeichan commented Apr 11, 2019

whymarrh commented Apr 12, 2019

chikeichan Apr 12, 2019

danjm Apr 12, 2019

danjm left a comment

danjm Apr 12, 2019

chikeichan Apr 16, 2019

danjm Apr 12, 2019

chikeichan Apr 16, 2019

danjm Apr 12, 2019

chikeichan Apr 16, 2019

danjm Apr 12, 2019

chikeichan Apr 16, 2019

danjm Apr 12, 2019

cjeria commented Apr 15, 2019

cjeria commented Apr 15, 2019

chikeichan commented Apr 16, 2019

danjm commented Apr 16, 2019

chikeichan commented Apr 17, 2019

bdresser commented Apr 17, 2019

danjm commented Apr 17, 2019

cjeria commented Apr 17, 2019

bdresser commented Apr 17, 2019 •

edited

Loading

danjm commented Apr 17, 2019

danjm left a comment

Add token selection to the send screen #6445

Add token selection to the send screen #6445

Conversation

chikeichan commented Apr 11, 2019

whymarrh commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjm 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

cjeria commented Apr 15, 2019

cjeria commented Apr 15, 2019

chikeichan commented Apr 16, 2019

danjm commented Apr 16, 2019

chikeichan commented Apr 17, 2019

bdresser commented Apr 17, 2019

danjm commented Apr 17, 2019

cjeria commented Apr 17, 2019

bdresser commented Apr 17, 2019 • edited Loading

danjm commented Apr 17, 2019

danjm left a comment

Choose a reason for hiding this comment

bdresser commented Apr 17, 2019 •

edited

Loading