New settings custom rpc form #6490

danjm · 2019-04-22T16:50:52Z

resolves #6209

This PR implements the new designs for the Custom RPC form. This PR incorporates everything that was included in the previous #6483 PR, and included additions of the customizable block explorer. It should also incorporate fixes to all the feedback from @bdresser and @cjeria

demo:

ui/app/components/app/modals/account-details-modal.js

app/_locales/en/messages.json

ui/app/pages/settings/networks-tab/networks-tab.component.js

metamaskbot · 2019-04-25T19:17:05Z

Builds ready [17f976f]: chrome, firefox, edge, opera

metamaskbot · 2019-04-29T06:37:31Z

Builds ready [6ddb24f]: chrome, firefox, edge, opera

tmashuang · 2019-05-06T22:15:10Z

ui/app/pages/settings/networks-tab/network-form/network-form.component.js

+            } else {
+              setRpcTarget(rpcUrl, chainId, ticker, networkName, {
+                blockExplorerUrl: blockExplorerUrl || rpcPrefs.blockExplorerUrl,
+                ...rpcPrefs,


This isn't actually setting the blockExplorerUrl in rpcPrefs, which is being passed to setRpcTarget in the background as undefined.

@tmashuang So when you submit a custom rpc form with a block explorer url, you don't have anything set in any of the rpc objects in the state.metamask.frequentRpcListDetail array?

No, it's not there either. I was looking at it from the provider params, state.metamask.provider when the network is selected.

@tmashuang This should be fixed now.

metamaskbot · 2019-05-07T18:34:33Z

Builds ready [0eda003]: chrome, firefox, edge, opera

tmashuang · 2019-05-08T19:58:49Z

app/scripts/metamask-controller.js

-    await this.preferencesController.updateRpc({ rpcUrl, chainId, ticker, nickname })
-    this.networkController.setRpcTarget(rpcUrl, chainId, ticker, nickname)
+  async updateAndSetCustomRpc (rpcUrl, chainId, ticker = 'ETH', nickname, rpcPrefs) {
+    console.log('updateAndSetCustomRpc rpcUrl, chainId, ticker, nickname, rpcPrefs', rpcUrl, chainId, ticker, nickname, rpcPrefs)


console.log here

Fixed in 0707b27

tmashuang · 2019-05-08T19:59:20Z

app/scripts/controllers/preferences.js

-      if (!!chainId && !Number.isNaN(parseInt(chainId))) {
-        checkedChainId = chainId
+    addToFrequentRpcList (url, chainId, ticker = 'ETH', nickname = '', rpcPrefs = {}) {
+      console.log('addToFrequentRpcList url, chainId, ticker, nickname, rpcPrefs', url, chainId, ticker, nickname, rpcPrefs)


console.log here

fixed in 0707b27

tmashuang · 2019-05-08T19:59:29Z

ui/app/pages/settings/networks-tab/networks-tab.container.js

+  return {
+    setSelectedSettingsRpcUrl: newRpcUrl => dispatch(setSelectedSettingsRpcUrl(newRpcUrl)),
+    setRpcTarget: (newRpc, chainId, ticker, nickname, rpcPrefs) => {
+      console.log('setRpcTarget newRpc, chainId, ticker, nickname, rpcPrefs', newRpc, chainId, ticker, nickname, rpcPrefs)


console.log here

fixed in 0707b27

tmashuang · 2019-05-08T19:59:31Z

ui/app/pages/settings/networks-tab/networks-tab.container.js

+    displayWarning: warning => dispatch(displayWarning(warning)),
+    setNetworksTabAddMode: isInAddMode => dispatch(setNetworksTabAddMode(isInAddMode)),
+    editRpc: (oldRpc, newRpc, chainId, ticker, nickname, rpcPrefs) => {
+      console.log('editRpc oldRpc, newRpc, chainId, ticker, nickname, rpcPrefs', oldRpc, newRpc, chainId, ticker, nickname, rpcPrefs)


console.log here

fixed in 0707b27

tmashuang · 2019-05-08T19:59:38Z

ui/app/store/actions.js

  return (dispatch) => {
    log.debug(`background.updateAndSetCustomRpc: ${newRpc} ${chainId} ${ticker} ${nickname}`)
-    background.updateAndSetCustomRpc(newRpc, chainId, ticker, nickname || newRpc, (err, result) => {
+    console.log('updateAndSetCustomRpc newRpc, chainId, ticker, nickname || newRpc, rpcPrefs', newRpc, chainId, ticker, nickname || newRpc, rpcPrefs)


console.log here

fix during rebase

whymarrh · 2019-05-08T19:59:59Z

Updated to resolve a conflict from #6583 (commit will be removed in the squash+merge anyhow)

tmashuang · 2019-05-08T20:15:12Z

ui/app/components/app/modals/account-details-modal.js

+        onClick: () => {
+          global.platform.openWindow({ url: genAccountLink(address, network, rpcPrefs) })
+        },
+      }, this.context.t('blockExplorerView', rpcPrefs.blockExplorerUrl && [rpcPrefs.blockExplorerUrl])),


I am getting View account at $1 on the modal screen when on the default Ethereum networks. Could we do

Suggested change

}, this.context.t('blockExplorerView', rpcPrefs.blockExplorerUrl && [rpcPrefs.blockExplorerUrl])),

}, rpcrpcPrefs.blockExplorerUrl ? this.context.t('blockExplorerView [rpcPrefs.blockExplorerUrl])) : this.context.t('viewOnEtherscan'),

and with [rpcPrefs.blockExplorerUrl] could we parse the url with Regex for just the domain? For my first ugly stab at it,

const expression = /(\w+\.)/gi const regex = new RegExp(expression) customBlockExplorer.match(regex)[0].charAt(0).toUpperCase() + customBlockExplorer.match(regex)[0].slice(1).replace(/[^a-zA-Z ]/gi, '')

Edit: probably this [^https?\:\/\/]\w+ might be simpler but doesn't catch some edge cases.

Also, needs the same logic for

metamask-extension/ui/app/components/app/dropdowns/account-details-dropdown.js

Line 118 in 3117562

text: this.context.t('viewOnEtherscan'),

Fixed in ed81fed

tmashuang · 2019-05-08T20:16:11Z

app/_locales/en/messages.json

+    "message": "Block Explorer"
+  },
+  "blockExplorerView": {
+    "message": "View account at $1",


at vs on?

Suggested change

"message": "View account at $1",

"message": "View account on $1",

@danjm addressed a similar comment below and I believe $1 will also be a URL here:

I don't know what the correct grammar is here. $1 will always be a [URL]. "View at http://example.com" sounds a bit more correct... but I don't know why and I might be wrong.

I believe that Dan's correct here and that "at" sounds more natural.

whymarrh

A few small comments and a few quick suggestions

whymarrh · 2019-05-09T13:27:52Z

app/_locales/en/messages.json

+  },
+  "blockExplorerView": {
+    "message": "View account at $1",
+    "description": "$1 replaced by url for custom block explorer"


Suggested change

"description": "$1 replaced by url for custom block explorer"

"description": "$1 replaced by URL for custom block explorer"

fixed in 5083384

whymarrh · 2019-05-09T13:28:19Z

app/_locales/en/messages.json

@@ -245,6 +258,9 @@
  "cancelN": {
    "message": "Cancel all $1 transactions"
  },
+  "chainId": {
+    "message": "Chain Id"


Suggested change

"message": "Chain Id"

"message": "Chain ID"

fixed in 5083384

whymarrh · 2019-05-09T13:30:11Z

app/_locales/en/messages.json

+    "message": "Block Explorer"
+  },
+  "blockExplorerView": {
+    "message": "View account at $1",


@danjm addressed a similar comment below and I believe $1 will also be a URL here:

I don't know what the correct grammar is here. $1 will always be a [URL]. "View at http://example.com" sounds a bit more correct... but I don't know why and I might be wrong.

I believe that Dan's correct here and that "at" sounds more natural.

whymarrh · 2019-05-09T13:34:56Z

ui/app/helpers/utils/transactions.util.js

@@ -188,3 +190,17 @@ export function getStatusKey (transaction) {

  return transaction.status
 }
+
+/**
+ * An external block explorer url at which a transaction can be viewed.


Suggested change

* An external block explorer url at which a transaction can be viewed.

* Returns an external block explorer URL at which a transaction can be viewed.

fixed in 29c5762

whymarrh · 2019-05-09T13:35:39Z

ui/app/pages/settings/networks-tab/network-form/network-form.component.js

+
+export default class NetworksTab extends PureComponent {
+  static contextTypes = {
+    t: PropTypes.func,


Suggested change

t: PropTypes.func,

t: PropTypes.func.isRequired,

I think we should try to add .isRequired to all appropriate prop types going forward. I've suggested them here for the contextTypes only since I know that these are required and suggesting it for the correct props would require more thought—if you want to add them quickly go for it, but this is more a "moving forward" suggestion.

fixed in 29c5762

whymarrh · 2019-05-09T13:35:48Z

ui/app/pages/settings/networks-tab/network-form/network-form.component.js

+export default class NetworksTab extends PureComponent {
+  static contextTypes = {
+    t: PropTypes.func,
+    metricsEvent: PropTypes.func,


Suggested change

metricsEvent: PropTypes.func,

metricsEvent: PropTypes.func.isRequired,

See note above (attached to t)

fixed in 29c5762

whymarrh · 2019-05-09T13:43:53Z

ui/app/pages/settings/networks-tab/network-form/network-form.component.js

+      this.setErrorTo(stateKey, '')
+    } else {
+      const appendedRpc = `http://${url}`
+      const validWhenAppended = validUrl.isWebUri(appendedRpc) && !url.match(/^https*:\/\/$/)


Suggested change

const validWhenAppended = validUrl.isWebUri(appendedRpc) && !url.match(/^https*:\/\/$/)

const validWhenAppended = validUrl.isWebUri(appendedRpc) && !url.match(/^https?:\/\/$/)

This regex will allow zero or more 's' characters following 'http' (i.e., *) when it should be zero or 1 (i.e., ?).^[1]

fixed in 29c5762

whymarrh · 2019-05-09T13:47:32Z

ui/app/pages/settings/networks-tab/networks-tab.component.js

+
+export default class NetworksTab extends PureComponent {
+  static contextTypes = {
+    t: PropTypes.func,


Suggested change

t: PropTypes.func,

t: PropTypes.func.isRequired,

fixed in 29c5762

whymarrh · 2019-05-09T13:47:43Z

ui/app/pages/settings/networks-tab/networks-tab.component.js

+export default class NetworksTab extends PureComponent {
+  static contextTypes = {
+    t: PropTypes.func,
+    metricsEvent: PropTypes.func,


Suggested change

metricsEvent: PropTypes.func,

metricsEvent: PropTypes.func.isRequired,

fixed in 29c5762

whymarrh · 2019-05-09T13:53:14Z

Also with #6583 merged, a few functions might need to be updated

… been updated.

metamaskbot · 2019-05-09T15:23:37Z

Builds ready [039acca]: chrome, firefox, edge, opera

…ixes on network tab and form.

metamaskbot · 2019-05-09T16:09:40Z

Builds ready [29c5762]: chrome, firefox, edge, opera

whymarrh · 2019-05-09T16:12:54Z

LGTM

danjm requested a review from whymarrh as a code owner April 22, 2019 16:50

danjm changed the base branch from develop to custom-rpc-form-updates April 22, 2019 16:51

danjm force-pushed the custom-rpc-form-updates branch from d523d9d to 817f4c2 Compare April 22, 2019 16:54

chikeichan reviewed Apr 24, 2019

View reviewed changes

ui/app/components/app/modals/account-details-modal.js Outdated Show resolved Hide resolved

app/_locales/en/messages.json Show resolved Hide resolved

ui/app/pages/settings/networks-tab/networks-tab.component.js Outdated Show resolved Hide resolved

danjm mentioned this pull request Apr 29, 2019

New custom rpc form designs #6483

Closed

danjm changed the title ~~Add block explorer customization to network form~~ New settings custom rpc form Apr 29, 2019

danjm changed the base branch from custom-rpc-form-updates to develop April 29, 2019 21:40

tmashuang reviewed May 6, 2019

View reviewed changes

danjm force-pushed the custom-block-explorers branch 2 times, most recently from 9b67e59 to 0eda003 Compare May 7, 2019 18:19

tmashuang reviewed May 8, 2019

View reviewed changes

whymarrh suggested changes May 9, 2019

View reviewed changes

danjm added 7 commits May 9, 2019 11:58

Add networks tab to settings, with header.

a657c83

Adds network list to settings network tab.

8884294

Adds form to settings networks tab and connects it to network list.

14af59d

Network tab: form adding and editing working

6c03914

Settings network form properly handles input errors

f08bfa0

Add translations for settings network form

7ab6273

Clean up styles of settings network tab.

79816fe

danjm added 17 commits May 9, 2019 11:59

Hide cancel button.

dd906b0

Highlight and show network form of provider by default.

3eede4a

Standardize network subheader name to 'Networks'

27aef96

Update e2e tests for new settings network form

e977efb

Update unit tests for new rpcPrefs prop

1a40f77

Extract blockexplorer url construction into method.

39ee3fc

Fix broken styles on non-network tabs in popup mode

56e8e87

Fix block explorer url links for cases when provider in state has not…

e74f38b

… been updated.

Fix vertical spacing of network form

06c09b2

Don't allow click of save button on network form if nothing has changed

af04132

Ensure add network button is shown in popup view

e979446

Lint fix for networks tab

fc491d8

Fix block explorer url preference setting.

d002157

Fix e2e tests for custom blockexplorer in account details modal changes.

42016c6

Update integration test states to include frequentRpcList property

4fa5146

Fix some capitalizations in en/messages.json

5083384

Remove some console.logs added during custom rpc form work

0707b27

danjm force-pushed the custom-block-explorers branch from 5a29eee to 039acca Compare May 9, 2019 15:10

danjm added 2 commits May 9, 2019 13:11

Fix external account link text and url for modal and dropdown.

ed81fed

Documentation, url validation, proptype required additions and lint f…

29c5762

…ixes on network tab and form.

danjm force-pushed the custom-block-explorers branch from 039acca to 29c5762 Compare May 9, 2019 15:42

whymarrh approved these changes May 9, 2019

View reviewed changes

danjm merged commit 13be683 into develop May 9, 2019

whymarrh deleted the custom-block-explorers branch May 9, 2019 18:46

This was referenced May 23, 2019

Allow custom block explorer #6083

Closed

CustomRPC settings: URL box should be on the top #5605

Closed

Custom Network pointing at Rinkeby Etherscan. #6127

Closed

	}, this.context.t('blockExplorerView', rpcPrefs.blockExplorerUrl && [rpcPrefs.blockExplorerUrl])),
	}, rpcrpcPrefs.blockExplorerUrl ? this.context.t('blockExplorerView [rpcPrefs.blockExplorerUrl])) : this.context.t('viewOnEtherscan'),

	"message": "View account at $1",
	"message": "View account on $1",

	"description": "$1 replaced by url for custom block explorer"
	"description": "$1 replaced by URL for custom block explorer"

	* An external block explorer url at which a transaction can be viewed.
	* Returns an external block explorer URL at which a transaction can be viewed.

	metricsEvent: PropTypes.func,
	metricsEvent: PropTypes.func.isRequired,

	const validWhenAppended = validUrl.isWebUri(appendedRpc) && !url.match(/^https*:\/\/$/)
	const validWhenAppended = validUrl.isWebUri(appendedRpc) && !url.match(/^https?:\/\/$/)

New settings custom rpc form #6490

New settings custom rpc form #6490

Conversation

danjm commented Apr 22, 2019 • edited Loading

metamaskbot commented Apr 25, 2019

metamaskbot commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmashuang May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented May 7, 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

whymarrh commented May 8, 2019

tmashuang May 8, 2019 • edited Loading

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 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

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

whymarrh commented May 9, 2019

metamaskbot commented May 9, 2019

metamaskbot commented May 9, 2019

whymarrh commented May 9, 2019

danjm commented Apr 22, 2019 •

edited

Loading

tmashuang May 6, 2019 •

edited

Loading

tmashuang May 8, 2019 •

edited

Loading