Refactor and redesign confirm transaction views #4691

alextsg · 2018-06-29T04:38:02Z

No description provided.

danjm · 2018-06-30T00:47:11Z

@alextsg What are your thoughts on adding directory specific .selectors files for most of the components/pages/confirm-** directories, to be used to replace any selection logic within the mapStateToProps functions?

My preference is for no selection logic at all in mapStateToProps, and instead contain all of that in imported selector functions. Also, if there are selectors that will only be used in one container component, my preference is that they be in a file in the same directory as that component.

Thoughts?

danjm · 2018-06-30T01:01:35Z

ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js

+    t: PropTypes.func,
+  }
+
+  static propTypes = {


Perhaps alphabetize this list and put spaces between the sections (by type of source) of props.

danjm · 2018-06-30T01:15:42Z

ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js

+      } = {},
+    } = this.props
+
+    const INSUFFICIENT_FUNDS_ERROR = t('insufficientFunds')


I think it might be better to store errors as their translation keys, as opposed to their translations. And then translate them at the point of render.

Reasons:
(1) Data stored will not be a 'view' value, just data that represents the current state of errors
(2) Debugging will be easier if the users state logs are all in english, as opposed to some parts of state that representing error handling being in a different laguage

Also, perhaps these errors should be moved to a constants file

danjm · 2018-06-30T01:18:37Z

ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js

+      return
+    }
+  }
+


What are your thoughts on moving some or all of the logic in getError and validateEditGas to either reducers or utils?

I think keeping errors in the redux state makes sense when there's an app-wide display for errors (like how GitHub has it under the nav bar) but when the error is localized to a component then I don't think it's necessary, since it's basically up to the component how the error should be handled (displayed somewhere, disable buttons, etc.).

As for utils, I get that by doing so it keeps the component file smaller, but in my opinion the purpose of a util is re-usability. I want to avoid passing in a bunch of arguments into a util function as that just makes the util very specific and hard to use. In this case, I think isBalanceSufficient is great as a util, but getError would be too specific to the component. validateEditGas could probably be moved to the container file, but we should figure out how to deal with the translation of error messages.

danjm · 2018-06-30T01:21:08Z

ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js

+    if (onEditGas) {
+      onEditGas()
+    } else {
+      showCustomizeGasModal({


Maybe put all of showCustomizeGasModal in the container?

... maybe not

danjm · 2018-06-30T12:09:41Z

app/scripts/controllers/transactions/tx-state-manager.js

@@ -399,19 +399,17 @@ class TransactionStateManager extends EventEmitter {
  _setTxStatus (txId, status) {
    const txMeta = this.getTx(txId)
    txMeta.status = status


Why is this change needed?

danjm · 2018-06-30T12:12:33Z

ui/app/actions.js

@@ -705,11 +705,10 @@ function signTypedMsg (msgData) {

 function signTx (txData) {
  return (dispatch) => {
-    dispatch(actions.showLoadingIndication())


Why do you remove showLoadingIndication and hideLoadingIndication here, but not in the other methods edited below?

This is a weird case where the callback passed to ethQuery.sendTransaction gets called when the tx gets approved or rejected, and not when we'd expect (called after the tx is added to the list of unapproved tx'es). The loading spinner was being hidden unintentionally during approving/rejecting on the confirm step.

danjm · 2018-06-30T12:15:11Z

ui/app/actions.js

    })
+    .then(() => updateMetamaskStateFromBackground())


Not related to this PR, but we should probably rename updateMetamaskStateFromBackground to something like getUpdatedMetamaskStateFromBackground. The current name makes it sound like the method does the updating of react state itself.

Leaving a TODO here for myself to do this after I finish reviwing:

create github issue to rename updateMetamaskStateFromBackground

danjm · 2018-07-01T12:07:19Z

ui/app/selectors/confirm-transaction.js

@@ -0,0 +1,65 @@
+import { createSelector } from 'reselect'


Do you prefer to have these here in a general selectors/ directory, as opposed to in one of the confirm-transaction directories? (As is done, for example, with send)

danjm · 2018-07-01T17:55:23Z

ui/app/components/tabs/tabs.component.js

+
+  renderTabs () {
+    // const { children } = this.props
+    const numberOfTabs = React.Children.count(this.props.children)


Out of curiosity why is React.Children (and not this.props.children) needed here?

remove commented out code

React.Children is a set of utils that can be used on this.props.children https://reactjs.org/docs/react-api.html#reactchildren

danjm · 2018-07-01T18:28:41Z

...rm-page-container/confirm-page-container-content/confirm-page-container-content.component.js

+    const { detailsComponent, dataComponent } = this.props
+
+    return (
+      <Tabs>


It might be nice if the tabs component had an API that allowed:

<Tabs tabs={ 'Details': detailsComponent, 'Data': dataComponent, } />

I was actually trying to avoid this so that the elements would be more HTML semantic:

<Tabs> <Tab /> <Tab /> </Tabs>

danjm · 2018-07-01T18:30:09Z

...rm-page-container/confirm-page-container-content/confirm-page-container-content.component.js

+        { this.renderContent() }
+        {
+          errorMessage && (
+            <div className="confirm-page-container-content__error-container">


Should we put the <div className="confirm-page-container-content__error-container"> inside the ConfirmPageContainerError component?

Not here, because the confirm-page-container-content__error-container class adds padding around the ConfirmPageContainerError component. This makes the ConfirmPageContainerError more reusable and contained since the parent would be responsible for the spacing of the child. I might not want the same spacing/padding used elsewhere.

danjm · 2018-07-01T18:35:46Z

...container-content/confirm-page-container-summary/confirm-page-container-summary.component.js

+        </div>
+      </div>
+      {
+        hideSubtitle || <div className="confirm-page-container-summary__subtitle">


It might be slight improvement on code readability if this was showSubtitle && . I say this because the other conditionally rendered elements are rendered if a value is truthy with &&. Reading the file and then encountering || is a little confusing. I say this lightly though, if switching to showSubtitle && makes things less readable or intuitive elsewhere, then my suggestion here should be ignored.

I did it this way because the default of the summary component is to show the subtitle, and hiding it is the exception. If I used showSubtitle as the boolean prop then I think it's a less convenient API since I'd have to add that in more places than not.

danjm · 2018-07-02T14:54:53Z

ui/app/components/modals/modal.js

@@ -267,7 +269,31 @@ const MODALS = {

  CUSTOMIZE_GAS: {
    contents: [


I guess we also need to switch over the modal used on the send screen at some point (but not as part of this PR).

Create github issue for using new customize gas modal component on send screen.

danjm · 2018-07-02T15:07:49Z

ui/app/components/pages/confirm-transaction-switch/confirm-transaction-switch.container.js

+import ConfirmTransactionSwitch from './confirm-transaction-switch.component'
+
+const mapStateToProps = state => {
+  const { confirmTransaction } = state


Perhaps we should map the specific properties within confirmTransaction to props here. It will simplify the logic within the component.

danjm · 2018-07-02T15:09:12Z

ui/app/components/pages/confirm-transaction-switch/confirm-transaction-switch.util.js

@@ -0,0 +1,4 @@
+export function isConfirmDeployContract (txData = {}) {
+  const { txParams = {} } = txData
+  return !txParams.to


This is consistent with current code, but I wonder if there is a more specific check. It seems likely that we will be handling other transactions in the future that don't have a recipient address. Perhaps we can parse something from data?

The tools I have for parsing data (abi-decoder and eth-method-registry) don't return anything from the deploy contract data, so I don't think there's a quick fix at our disposal. I asked kumavis about this as well and deploying a contract should be the only case when txParams.to is not defined.

danjm · 2018-07-02T15:11:14Z

ui/app/components/pages/confirm-transaction/confirm-transaction.component.js

+
+    if (paramsTransactionId) {
+      // Check to make sure params ID is valid
+      const tx = R.find(({ id }) => id + '' === paramsTransactionId)(unconfirmedTransactions)


I'm fine with using ramda, but the code might become a little more accessible if we use the native find method.

danjm · 2018-07-02T15:13:32Z

ui/app/components/pages/confirm-transaction/confirm-transaction.component.js

+    }
+  }
+
+  setTransactionToConfirm () {


I think this method is another case where the logic could be moved to the .container component and then to selectors.

I kept this method in the component file since it works with react-router props. I kinda see methods in the container file as having to do with modifying the data the component uses and not anything to do with the view, so I've been keeping react-router/view changes in the component and not the container.

danjm · 2018-07-02T15:16:01Z

ui/app/components/send_/send.utils.js

@@ -45,9 +45,9 @@ function calcGasTotal (gasLimit, gasPrice) {

 function isBalanceSufficient ({
  amount = '0x0',
-  amountConversionRate = 0,


I wonder why amountConversionRate was defaulted to 0 before? We should confirm that changing the default to 1 does not break any desired behaviour.

danjm · 2018-07-02T15:18:59Z

ui/app/conversion-util.js

@@ -28,6 +28,8 @@ const BN = ethUtil.BN
 const R = require('ramda')
 const { stripHexPrefix } = require('ethereumjs-util')

+global.BigNumber = BigNumber


Why add BigNumber to global?

danjm · 2018-07-02T15:23:50Z

ui/app/helpers/confirm-transaction/util.js

@@ -0,0 +1,116 @@
+import currencyFormatter from 'currency-formatter'


I asked a similar question elsewhere. Your thoughts on including this in a general ui/app/helpers/ directory as opposed to within a root directory for confirm-transaction components?

I just found myself reusing these in various components and the .duck, and it got annoying having to move these functions/files around based on which component was using a function during debugging/refactoring (up one level when used by a parent, down one level when not used).

alextsg · 2018-07-02T22:24:56Z

@danjm I'm indifferent to using selectors as it has pros and cons, unless it's for more complicated retrieval or using a combination of selectors with reselect that can benefit from memoization. While it can make updating state easier since you just need to update the selector, realistically that's kinda rare and you'd likely have to update all the selector function names anyways. Your mapStateToProps functions in the send containers look neat, but it makes it so that your container files provide very little context as to what properties are being pulled since all you see are function names (especially if props nested differently have the same names). I also find it annoying to have to navigate between a bunch of files when debugging.

As for the folder structure, I think I placed selectors I did use in a top level selectors folder, since I used them in both a duck file and container files if I remember correctly.

…ests.

…clicking through. (e2e beta tests)

…n to contain localhost before attempting to click. (e2e beta tests)

…ered before attempting to select the tx-list-item. (e2e beta tests)

…e-upgrades e2e beta test upgrades for confirm refactor

…firmations that have data

metamaskbot · 2018-07-12T00:47:16Z

Builds ready [6701771]: mascara, chrome, firefox, edge, opera

alextsg · 2018-07-12T16:39:25Z

This closes #4298 closes #4635 closes #3414 closes #4404 closes #4481 closes #4429 closes #3824 closes #4430

1blockologist · 2018-08-02T18:48:25Z

What is the process of getting this up into master/production? Didn't see a place to chat with the team (maybe add chat server badge to readme?)

bdresser · 2018-08-02T19:36:51Z

@1blockologist this will bei n 4.9.0, should be out early next week! thanks for your patience.

abitrolly · 2018-08-12T15:39:42Z

Where are screenshots of before and after? Common, it is just Ctrl+Shift+PrtSc and Ctrl+V on GNOME. ;)

alextsg force-pushed the i4404-confirm-refactor branch from ce7adb1 to 42ada41 Compare June 29, 2018 04:50

danjm reviewed Jun 30, 2018

View reviewed changes

danjm reviewed Jul 1, 2018

View reviewed changes

danjm reviewed Jul 2, 2018

View reviewed changes

whymarrh mentioned this pull request Jul 5, 2018

Inline simple selectors used in mapStateToProps #4736

Closed

alextsg force-pushed the i4404-confirm-refactor branch 2 times, most recently from 7380c90 to c123419 Compare July 6, 2018 19:06

alextsg added 4 commits July 6, 2018 16:27

Refactor and redesign confirm transaction views

ea9d51e

Refactor data flow, signature request routing

e895498

Display newest transaction

7732cd4

Use eth-method-registry to get method data

9cde5ab

alextsg force-pushed the i4404-confirm-refactor branch from c123419 to a2d9c43 Compare July 6, 2018 23:27

alextsg and others added 13 commits July 7, 2018 18:53

Fix existing unit tests

7d691c7

Add unit tests for confirmTransaction duck and util

f6e90bb

Fix multiple signature requests showing an infinite spinner

76e80f6

Handles window indeterminancy issues for external dapps in e2e beta t…

1a0a701

…ests.

Fix existing e2e tests

228b807

Update e2e beta tests to cover confirm refactor changes.

f81676f

Fix signature request tests for compatability with confirm refactor.

a7a6617

Fix send integration tests for compatability with confirm refactor.

91eca55

Fix bug in test mock signature request methods.

e91d87e

Wait for submitted tx list item amounts to match the expected before …

8912955

…clicking through. (e2e beta tests)

Reattempt to find network selector; wait for network selector dropdow…

05ee2b7

…n to contain localhost before attempting to click. (e2e beta tests)

Wait for an element with a .tx-list-pending-item-container to be rend…

07322bc

…ered before attempting to select the tx-list-item. (e2e beta tests)

Merge pull request #4764 from MetaMask/i4404-confirm-refactor-with-e2…

92fc87a

…e-upgrades e2e beta test upgrades for confirm refactor

danjm previously approved these changes Jul 11, 2018

View reviewed changes

Fix font sizes, colors. Include data tab for send-eth transaction con…

6701771

…firmations that have data

alextsg dismissed danjm’s stale review via 6701771 July 12, 2018 00:35

danjm approved these changes Jul 12, 2018

View reviewed changes

alextsg merged commit 0d4dbbe into develop Jul 12, 2018

alextsg deleted the i4404-confirm-refactor branch July 12, 2018 01:31

bdresser mentioned this pull request Jul 12, 2018

Feature request: showing the name and params of a contract method that's being called #2152

Closed

alextsg mentioned this pull request Jul 16, 2018

Tests for confirm screen refactor #4679

Closed

This was referenced Jul 19, 2018

Cannot change gas price on deploying contracts #4831

Closed

New UI: Missing transaction data #3367

Closed

brad-decker mentioned this pull request Jun 28, 2021

remove ui lookup of lastGasPrice field #11411

Merged

		@@ -267,7 +269,31 @@ const MODALS = {

		CUSTOMIZE_GAS: {
		contents: [

		@@ -0,0 +1,116 @@
		import currencyFormatter from 'currency-formatter'

Refactor and redesign confirm transaction views #4691

Refactor and redesign confirm transaction views #4691

Conversation

alextsg commented Jun 29, 2018

danjm commented Jun 30, 2018

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

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

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

alextsg commented Jul 2, 2018

metamaskbot commented Jul 12, 2018

alextsg commented Jul 12, 2018

1blockologist commented Aug 2, 2018 • edited Loading

bdresser commented Aug 2, 2018

abitrolly commented Aug 12, 2018

1blockologist commented Aug 2, 2018 •

edited

Loading