registry dapp: cleanup, support reverse entries #3933

derhuerst · 2016-12-21T15:13:24Z

Fixes #3098, with the exception of removeReverse. Fixes #2933.

Work in progress, not happy with the actions & reducers boilerplate. Also the captions in the UI are not very helpful.

In preparation for the next commit.

coveralls · 2016-12-21T16:17:42Z

Changes Unknown when pulling 49e00ed on jr-registry-reverse into ** on master**.

coveralls · 2016-12-21T18:04:43Z

Changes Unknown when pulling 64c3ca5 on jr-registry-reverse into ** on master**.

coveralls · 2016-12-22T14:05:11Z

Coverage increased (+0.05%) to 85.77% when pulling 187a1c2 on jr-registry-reverse into be75cbf on master.

jacogr · 2016-12-22T15:47:16Z

js/src/dapps/registry/Accounts/accounts.js

    all: PropTypes.object.isRequired,
-    selected: PropTypes.object
+    selected: PropTypes.object,
+


Extra empty line for no immediately apparent reason.

props and actions separated.

jacogr · 2016-12-22T15:47:18Z

js/src/dapps/registry/Accounts/accounts.js

@@ -61,20 +65,27 @@ export default class Accounts extends Component {
  }

  renderAccount = (account) => {
-    const { all, selected } = this.props;
+    const { selected } = this.props;
    const isSelected = selected && selected.address === account.address;

    return (
      <MenuItem
        key={ account.address } value={ account.address }
        checked={ isSelected } insetChildren={ !isSelected }


Not related, however not sure why this stayed this way - everywhere we have an attribute per line

jacogr · 2016-12-22T15:47:21Z

js/src/dapps/registry/Accounts/accounts.js

+
+export default connect(
+  // mapStateToProps
+  (state) => state.accounts,


If we have to comment this to state intent, something is wrong, the code should just be self-explanatory (if the reader gets confused, it means we are not making the writing clear enough - a reason why in the rest of the app the functions are properly named)

jacogr · 2016-12-22T15:47:25Z

js/src/dapps/registry/Application/application.js

-    events: PropTypes.object.isRequired,
-    names: PropTypes.object.isRequired,
-    records: PropTypes.object.isRequired
+    fee: nullableProptype(PropTypes.object.isRequired)


Probably would make more sense as nullableProptype(PropTypes.object).isRequired ?

jacogr · 2016-12-22T15:47:34Z

js/src/dapps/registry/Events/events.css

+.inline {
+  display: inline-block;
+  width: auto;
+  marginRight: 1em;


margin-right?

jacogr · 2016-12-22T15:50:43Z

js/src/dapps/registry/Reverse/reverse.js

+              <MenuItem value='propose' primaryText='propose a reverse entry' />
+              <MenuItem value='confirm' primaryText='confirm a reverse entry' />
+            </DropDownMenu>
+            { action === 'propose' ? (


As per above, slightly more readable since there is a null for one condition, but the reader and maintainer gets lost here)

jacogr · 2016-12-22T15:50:47Z

js/src/dapps/registry/Reverse/reverse.js

+  onNameChange = (e) => {
+    this.setState({ name: e.target.value });
+  };
+  onAddressChange = (e) => {


Lines between functions. (All the ones below here could use the whitespace)

jacogr · 2016-12-22T15:50:50Z

js/src/dapps/registry/Reverse/reverse.js

+  };
+  onSubmitClick = () => {
+    const { action, name, address } = this.state;
+    if (action === 'propose') {


Blank line between variables and tests.

jacogr · 2016-12-22T15:50:54Z

js/src/dapps/registry/Reverse/reverse.js

+}
+
+export default connect(
+  // mapStateToProps


As per other places.

jacogr · 2016-12-22T15:50:58Z

js/src/dapps/registry/actions.js

+
+export const fetchIsTestnet = () => (dispatch) =>
+  api.net.version()
+  .then((netVersion) => {


Indent .then as per the rest of the codebase and the other files in this PR.

jacogr · 2016-12-22T15:51:02Z

js/src/dapps/registry/actions.js

+      netVersion === '3' // ropsten
+    ));
+  })
+  .catch((err) => {


.catch on same level as .then (not start of line)

jacogr · 2016-12-22T15:51:08Z

js/src/dapps/registry/ui/address.css

+  text-decoration: none;
+  color: inherit;
+}
+.link:hover {


Either use

.link { ... &:hover { ... } }

Or at the very least put blank lines before the blocks.

jacogr · 2016-12-22T15:51:11Z

js/src/dapps/registry/ui/address.css

+.link:hover {
+  text-decoration: underline;
+}
+.link abbr {


As per above comment. (PostCSS allows nesting)

jacogr · 2016-12-22T15:51:19Z

js/src/dapps/registry/ui/address.js

  return (
-    <div style={ container }>
-      <IdentityIcon address={ address } style={ align } />
+    <div key={ key } className={ styles.container }>


Single attribute per line when multiples.

jacogr · 2016-12-22T15:51:23Z

js/src/dapps/registry/ui/address.js

      { caption }
    </div>
  );
 };
+
+Address.propTypes = {


No static? This doesn't fit in with the rest of the codebase at all.

I guess this is not the place to discuss functional vs class components, so I will adapt it. 😉

jacogr · 2016-12-22T15:51:25Z

js/src/dapps/registry/ui/address.js

+  shortenHash: PropTypes.bool
+};
+
+Address.defaultProps = {


Same as per previous comment.

jacogr · 2016-12-22T15:51:29Z

js/src/dapps/registry/ui/hash.css

+  text-decoration: none;
+  color: inherit;
+}
+.link:hover {


PostCSS nesting or at least a blank line between blocks.

jacogr · 2016-12-22T15:51:34Z

js/src/dapps/registry/ui/hash.js

  return (<abbr title={ hash }>{ shortened }</abbr>);
 };
+
+Hash.propTypes = {


Let's do components in one way, here even in the PR we have different approaches. Consistency yields ease of maintainability.

jacogr · 2016-12-22T15:51:37Z

js/src/dapps/registry/ui/hash.js

+  linked: PropTypes.bool
+};
+
+Hash.defaultProps = {


As per above. Nothing wrong with simple components, but as soon as we start doing this we are adding yet another way of writing code.

jacogr · 2016-12-22T15:51:41Z

js/src/dapps/registry/util/etherscan-url.js

+const etherscanUrl = (hash, isTestnet) => {
+  hash = hash.toLowerCase().replace(leading0x, '');
+  const type = hash.length === 40 ? 'address' : 'tx';
+  return `https://${isTestnet ? 'testnet.' : ''}etherscan.io/${type}/0x${hash}`;


Blank like before return would be appreciated here.

jacogr · 2016-12-22T15:51:44Z

js/src/dapps/registry/util/post-tx.js

+      if (err && err.type === 'REQUEST_REJECTED') {
+        throw new Error('The request has been rejected.');
+      }
+      throw err;


Consider a blank line before.

derhuerst · 2016-12-22T16:51:51Z

Adressed most grumbles. For the rest, I either disagree or have questions.

coveralls · 2016-12-22T18:21:48Z

Coverage increased (+0.8%) to 86.515% when pulling 48ab2fe on jr-registry-reverse into c2e5b78 on master.

jacogr · 2016-12-25T08:59:15Z

Merge conflicts.

coveralls · 2016-12-26T14:25:35Z

Changes Unknown when pulling 66ef54b on jr-registry-reverse into ** on master**.

derhuerst added 14 commits December 21, 2016 15:49

style fixes ✨

b86d364

registry dapp: show reverse events

0e11780

registry dapp: actions & reducers for isTestnet

48fd48f

registry dapp: make Hash & Address components

4af4e93

registry dapp: code style ✨

6691eb3

registry dapp: bugfixes 🐛

b7a0f88

registry dapp: postTx helper

003b3ff

registry dapp: refactor reducers

c7cb7cc

registry dapp: use react-redux

98aae16

registry dapp: actions & reducers for reverse lookup

efae771

registry dapp: reverse lookup component

947754d

registry dapp: connect Address to redux

b95dc92

registry dapp: de-DRY recordTypeSelect

b8717b6

In preparation for the next commit.

registry dapp: support reverse lookup

49e00ed

derhuerst added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Dec 21, 2016

registry dapp: render reverse events

64c3ca5

derhuerst added 3 commits December 22, 2016 13:17

registry dapp: show tx sender, add key prop

b52721c

Merge master into jr-registry-reverse

3893bd7

registry dapp: link accounts to etherscan as well

187a1c2

derhuerst added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 22, 2016

jacogr reviewed Dec 22, 2016

View reviewed changes

js/src/dapps/registry/Events/events.css

.inline {

display: inline-block;

width: auto;

marginRight: 1em;

Copy link

Contributor

jacogr Dec 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

margin-right?

jacogr reviewed Dec 22, 2016

View reviewed changes

js/src/dapps/registry/Reverse/reverse.js

}

export default connect(

// mapStateToProps

Copy link

Contributor

jacogr Dec 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per other places.

jacogr reviewed Dec 22, 2016

View reviewed changes

js/src/dapps/registry/ui/address.js

shortenHash: PropTypes.bool

};

Address.defaultProps = {

Copy link

Contributor

jacogr Dec 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as per previous comment.

jacogr reviewed Dec 22, 2016

View reviewed changes

derhuerst added 2 commits December 22, 2016 17:46

merge master into jr-registry-reverse

9f3a562

registry dapp: address style grumbles 💄

48ab2fe

jacogr added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 25, 2016

derhuerst added 2 commits December 26, 2016 14:30

registry dapp: address style grumbles 💄

5ec9906

merge master into jr-registry-reverse

66ef54b

derhuerst added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Dec 26, 2016

gavofyork merged commit 002e8b0 into master Dec 27, 2016

gavofyork deleted the jr-registry-reverse branch December 27, 2016 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

registry dapp: cleanup, support reverse entries #3933

registry dapp: cleanup, support reverse entries #3933

derhuerst commented Dec 21, 2016 •

edited

Loading

coveralls commented Dec 21, 2016

coveralls commented Dec 21, 2016

coveralls commented Dec 22, 2016

jacogr Dec 22, 2016

derhuerst Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

derhuerst Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

derhuerst Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

derhuerst Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

jacogr Dec 22, 2016

derhuerst commented Dec 22, 2016

coveralls commented Dec 22, 2016

jacogr commented Dec 25, 2016

coveralls commented Dec 26, 2016

registry dapp: cleanup, support reverse entries #3933

registry dapp: cleanup, support reverse entries #3933

Conversation

derhuerst commented Dec 21, 2016 • edited Loading

coveralls commented Dec 21, 2016

coveralls commented Dec 21, 2016

coveralls commented Dec 22, 2016

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

derhuerst commented Dec 22, 2016

coveralls commented Dec 22, 2016

jacogr commented Dec 25, 2016

coveralls commented Dec 26, 2016

derhuerst commented Dec 21, 2016 •

edited

Loading