New signature request v3 UI #6891

tshfx · 2019-07-21T03:07:29Z

Work in progress

In this PR I've extracted the old Signature request component into a new component to follow newer practices.

Todo's

Finish styling
Add information tool tip

This work refers to #5388

danjm

This looks good overall. I've made a few suggestions here.

Once the styles are updated again according to the figma, I will do another review

danjm · 2019-07-23T16:29:56Z

ui/app/components/app/signature-request-original.js

@@ -273,8 +273,7 @@ SignatureRequest.prototype.renderBody = function () {
      }),
    }, [notice]),

-    h('div.request-signature__rows', type === 'eth_signTypedData' && version === 'V3' ?
-      this.renderTypedDataV3(data) :


Should delete the this.renderTypedDataV3 method too? No longer needed here, correct?

danjm · 2019-07-23T16:31:39Z

...ponents/app/signature-request/signature-request-header/signature-request-header.component.js

+      <div className="signature-request-header">
+        <div className="signature-request-header--account">
+          {selectedAccount && accounts && <AccountDropdownMini
+            selectedAccount={selectedAccount}


Note that this selectedAccount is the current account. I think you mentioned you weren't sure where to get that from state

...nents/app/signature-request/signature-request-message/signature-request-message.component.js

ui/app/components/app/signature-request/signature-request.container.js

ui/app/pages/confirm-transaction/conf-tx.js

danjm · 2019-10-24T14:56:09Z

I've made some updates. This now looks like:

whymarrh · 2019-10-30T15:49:13Z

ui/app/components/app/signature-request-original.js

+        if (typeof value === 'boolean') {
+          value = value.toString()
+        }
+        return h('div.request-signature__row', [


Should we put a key prop on each of these rows? Minimally we could be using the index?

whymarrh · 2019-10-30T15:54:16Z

...ponents/app/signature-request/signature-request-header/signature-request-header.component.js

+export default class SignatureRequestHeader extends PureComponent {
+  static propTypes = {
+    selectedAccount: PropTypes.object.isRequired,
+    accounts: PropTypes.object,


If this prop isn't required we should include a defaultProp for it

whymarrh · 2019-10-30T15:54:25Z

...ponents/app/signature-request/signature-request-header/signature-request-header.component.js

+
+  render () {
+    const { selectedAccount } = this.props
+    console.log('selectedAccount', selectedAccount)


Nit: console.log

Fixed in c8120c6dac920b8a64d46fce781d099aa398c8b1

...nents/app/signature-request/signature-request-message/signature-request-message.component.js

whymarrh · 2019-10-30T15:55:45Z

ui/app/components/app/signature-request/signature-request.component.js

+        address: PropTypes.string,
+        balance: PropTypes.string,
+      })
+    ),


If this prop isn't required, we should add a defaultProp for it

This is still pending

whymarrh · 2019-10-30T15:57:05Z

ui/app/pages/confirm-transaction/conf-tx.js

@@ -137,34 +138,46 @@ ConfirmTxScreen.prototype.getTxData = function () {
    : unconfTxList[index]
 }

+ConfirmTxScreen.prototype.signatureSelect = function (type, version) {
+  // Temporarily direct only v3 and v4 requests to new code.
+  console.log('type, version', type, version)


Nit: console.log

whymarrh · 2019-10-30T15:57:22Z

ui/app/pages/confirm-transaction/conf-tx.js

-      cancelTypedMessage: this.cancelTypedMessage.bind(this, txData),
-    })
-    : h(Loading)
+  console.log('msgParams', msgParams)


Nit: console.log

whymarrh · 2019-10-30T15:57:36Z

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

@@ -118,7 +119,7 @@ export default class ConfirmTransaction extends Component {
    // Show routes when state.confirmTransaction has been set and when either the ID in the params
    // isn't specified or is specified and matches the ID in state.confirmTransaction in order to
    // support URLs of /confirm-transaction or /confirm-transaction/<transactionId>
-
+    console.log('transactionId, paramsTransactionId', transactionId, paramsTransactionId)


Nit: console.log

whymarrh · 2019-10-30T15:57:48Z

ui/app/pages/home/home.component.js

@@ -97,7 +97,7 @@ export default class Home extends PureComponent {
      showRestorePrompt,
      threeBoxLastUpdated,
    } = this.props
-
+    console.log(123456)


Nit: console.log

whymarrh · 2019-10-30T15:57:55Z

ui/index.js

@@ -60,6 +60,7 @@ async function startApp (metamaskState, backgroundConnection, opts) {
  const unapprovedTxsAll = txHelper(metamaskState.unapprovedTxs, metamaskState.unapprovedMsgs, metamaskState.unapprovedPersonalMsgs, metamaskState.unapprovedTypedMessages, metamaskState.network)
  const numberOfUnapprivedTx = unapprovedTxsAll.length
  if (numberOfUnapprivedTx > 0) {
+    console.log('unapprovedTxsAll', unapprovedTxsAll)


Nit: console.log

danjm · 2019-10-31T15:53:04Z

@whymarrh All comments (except the question about the whether the array label issue was resolved) were resolved in c8120c6dac920b8a64d46fce781d099aa398c8b1

I also fixed the styling of the component in full screen mode in d223915e1a02b9c8ce0905aa24e653f2a6db8705

Looking into that array label issue now

danjm · 2019-10-31T15:57:51Z

And I left a comment regarding the array label issue

whymarrh · 2019-10-31T19:14:43Z

...nents/app/signature-request/signature-request-message/signature-request-message.component.js

+  renderNode (data) {
+    return (
+      <div className="signature-request-message--node">
+        {this.makeNode(data).map(([ label, value ], i) => (


From the looks of it, makeNode does nothing more than return the result of Object.entries, so this could be a call to that method directly.

Suggested change

{this.makeNode(data).map(([ label, value ], i) => (

{Object.entries(data).map(([ label, value ], i) => (

And we should be able to remove makeNode since it's just the once instance

whymarrh · 2019-10-31T19:15:44Z

ui/app/components/app/signature-request/signature-request.component.js

+        address: PropTypes.string,
+        balance: PropTypes.string,
+      })
+    ),


This is still pending

whymarrh · 2019-10-31T19:17:36Z

ui/app/components/app/signature-request/signature-request.component.js

+    const { clearConfirmTransaction, cancel } = this.props
+    const { metricsEvent } = this.context
+    if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
+      window.onbeforeunload = event => {


As @Gudahtt has pointed out elsewhere (#7333 and #7335):

Generally it's better to use window.addEventListener('beforeunload', (event) => { ... }); instead of window.onbeforeunload because it avoids overwriting other onbeforeunload handlers

whymarrh · 2019-10-31T19:18:55Z

@danjm want to rebase this on the latest develop when you have a minute

…ponent.js

whymarrh · 2019-11-01T17:56:56Z

@danjm this build is still failing

whymarrh

LGTM

done

Gudahtt · 2019-11-04T14:53:05Z

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

@@ -45,6 +45,7 @@ export default class ConfirmTransaction extends Component {
    isTokenMethodAction: PropTypes.bool,
    fullScreenVsPopupTestGroup: PropTypes.string,
    trackABTest: PropTypes.bool,
+    conversionRate: PropTypes.number,


Sorry for the post-merge nitpick, but this prop appears to be unused (both here and in the container component)

* Refactoring signature-request out to a new component. Wip * Styling polish and a better message display. * Update signature request header to no longer use account dropdown mini * Clean up code and styles * Code cleanup for signature request redesign branch * Fix signature request design for full screen * Replace makenode with object.entries in signature-request-message.component.js * Remove unused accounts prop from signature-request.component.js * Use beforeunload instead of window.onbeforeunload in signature-request

The styles for the multi-notification component on the home screen were accidentally removed while resolving a merge conflict in #6891. Fixes #7460

The header component for the new Signature Request screen has an undeclared variable called `name` in it. This was present in the original implementation of this component in #6891. It's unclear what this was supposed to be, and it doesn't seem to reference anything that exists.

danjm self-assigned this Jul 22, 2019

danjm added this to the UI Sprint 17 [July 22] milestone Jul 22, 2019

danjm previously requested changes Jul 23, 2019

View reviewed changes

bdresser modified the milestones: UI Sprint 17 [July 22], UI Sprint 18 [Aug 5] Aug 5, 2019

BrendanChou reviewed Aug 16, 2019

View reviewed changes

ui/app/pages/confirm-transaction/conf-tx.js Outdated Show resolved Hide resolved

bdresser modified the milestones: UI Sprint 18 [Aug 5], UI Sprint 19 [Aug 19] Aug 19, 2019

bdresser modified the milestones: UI Sprint 19 [Aug 19], UI Sprint 20 [Sept 16] Sep 13, 2019

danjm modified the milestones: UI Sprint 20 [Sept 16], UI Sprint 22 [Oct 14] Oct 11, 2019

danjm force-pushed the 5388-new-signing-ui branch from e6159bf to 6eac8d3 Compare October 24, 2019 05:22

danjm marked this pull request as ready for review October 28, 2019 14:54

danjm requested review from Gudahtt and whymarrh as code owners October 28, 2019 14:54

bdresser modified the milestones: UI Sprint 22 [Oct 14], UI Sprint 23 Oct 28, 2019

whymarrh suggested changes Oct 30, 2019

View reviewed changes

danjm force-pushed the 5388-new-signing-ui branch from 6eac8d3 to d223915 Compare October 31, 2019 15:50

whymarrh suggested changes Oct 31, 2019

View reviewed changes

Terry Smith and others added 5 commits November 1, 2019 11:40

Refactoring signature-request out to a new component. Wip

d2df748

Styling polish and a better message display.

e6d5672

Update signature request header to no longer use account dropdown mini

1ec0d9c

Clean up code and styles

f02a508

Code cleanup for signature request redesign branch

7f4a439

danjm added 3 commits November 1, 2019 11:40

Fix signature request design for full screen

a6f3b3c

Replace makenode with object.entries in signature-request-message.com…

bf4bdb8

…ponent.js

Remove unused accounts prop from signature-request.component.js

00d5888

danjm force-pushed the 5388-new-signing-ui branch from fcde372 to 5ec78d9 Compare November 1, 2019 14:10

Use beforeunload instead of window.onbeforeunload in signature-request

2fe6133

danjm force-pushed the 5388-new-signing-ui branch from 5ec78d9 to 2fe6133 Compare November 1, 2019 19:55

whymarrh approved these changes Nov 3, 2019

View reviewed changes

danjm merged commit 57a2966 into MetaMask:develop Nov 4, 2019

Gudahtt reviewed Nov 4, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Gudahtt mentioned this pull request Nov 18, 2019

Import styles for showing multiple notifications #7461

Merged

Gudahtt added a commit that referenced this pull request Nov 18, 2019

Import styles for showing multiple notifications

273ec7b

The styles for the multi-notification component on the home screen were accidentally removed while resolving a merge conflict in #6891. Fixes #7460

bbondy mentioned this pull request Dec 16, 2019

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

Closed

tmashuang mentioned this pull request Feb 5, 2020

Implement new signing UI #5388

Closed

Gudahtt mentioned this pull request Apr 14, 2020

Remove undeclared name variable from render #8336

Merged

vicnaum mentioned this pull request Dec 30, 2022

primaryType is not shown during signTyped - users are signing things almost blindly #17076

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New signature request v3 UI #6891

New signature request v3 UI #6891

tshfx commented Jul 21, 2019 •

edited

Loading

danjm left a comment

danjm Jul 23, 2019

danjm Jul 23, 2019

danjm commented Oct 24, 2019

whymarrh Oct 30, 2019

whymarrh Oct 30, 2019

whymarrh Oct 30, 2019

danjm Oct 31, 2019

whymarrh Oct 30, 2019

whymarrh Oct 31, 2019

whymarrh Oct 30, 2019

whymarrh Oct 30, 2019

whymarrh Oct 30, 2019

whymarrh Oct 30, 2019

whymarrh Oct 30, 2019

danjm commented Oct 31, 2019

danjm commented Oct 31, 2019

whymarrh Oct 31, 2019

whymarrh Oct 31, 2019

whymarrh Oct 31, 2019

whymarrh Oct 31, 2019

whymarrh commented Oct 31, 2019

whymarrh commented Nov 1, 2019

whymarrh left a comment

Gudahtt Nov 4, 2019

This comment has been minimized.

	{this.makeNode(data).map(([ label, value ], i) => (
	{Object.entries(data).map(([ label, value ], i) => (

New signature request v3 UI #6891

New signature request v3 UI #6891

Conversation

tshfx commented Jul 21, 2019 • edited Loading

Work in progress

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

danjm commented Oct 24, 2019

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 Oct 31, 2019

danjm commented Oct 31, 2019

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

whymarrh commented Oct 31, 2019

whymarrh commented Nov 1, 2019

whymarrh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

tshfx commented Jul 21, 2019 •

edited

Loading