feat/enhancements #8

epiqueras · 2018-01-23T20:45:34Z

This PR hopefully finalizes the boilerplate standard for now.

Make app more generic to Ethereum.
Group common state (accounts, balance) into wallet.
Add tests to 100% coverage. Closes add tests (the goal is 100% coverage) #4
Add Identicon (react-blockies). Closes add react-blockies for the security icons #5

I also went around and standardized import/exports to follow a common standard (The index files in utils/components were causing Webpack to choke up when ran though jest or storybook because of circular dependencies):

import regular named exports directly from their file using destructuring import syntax
e.g.

import { name } from '.../utils/something'

import actions, selectors, and shapes using the * as syntax
e.g.

import * as somethingActions from '.../actions/something'

somethingActions.FETCH_SOMETHING
somethingActions.fetchSomething()

import * as somethingSelectors from '.../reducers/something'

somethingSelectors.somethingShape.isRequired
somethingSelectors.getSomething(state)

Add normalize.css.

Closes #5

Closes #4

coveralls · 2018-01-23T20:50:01Z

Pull Request Test Coverage Report for Build 12

61 of 61 (100.0%) changed or added relevant lines in 14 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+94.3%) to 95.098%

Totals
Change from base Build 7:	94.3%
Covered Lines:	126
Relevant Lines:	126

💛 - Coveralls

satello

Nice work! My biggest overarching concern is there is quite a bit of code in here that currently wouldn't be used in the DApp such as the all the eth balance stuff and some of the utils and helpers. If we think we will never use it its probably worth it to take it out to reduce the clutter

satello · 2018-01-23T21:31:49Z

README.md

-DEV_ARBITRATOR_ADDRESS=0xaea35f89f98996ae06aac344ab4b9ce1731059c4
+REACT_APP_DEV_ETHEREUM_PROVIDER=http://localhost:8545
+REACT_APP_DEV_STORE_PROVIDER=https://kleros.in
+REACT_APP_DEV_ARBITRATOR_ADDRESS=0xaea35f89f98996ae06aac344ab4b9ce1731059c4


This address has changed. I think you might just want to leave this blank in the boilerplate as it is likely to change a lot

satello · 2018-01-23T21:35:54Z

src/bootstrap/initializer.js

+import * as walletSelectors from '../reducers/wallet'
+import { eth } from './kleros'
+import { renderIf } from '../utils/react-redux'
+import RequiresMetaMask from '../components/requires-meta-mask'


I like components being PascalCase

Do you mean I should change the filename/path? Or is this good?

The only reason I made the filename kebab-case was so we can enforce a consistent filename pattern with the linter. Otherwise it would be up to us to make sure only component files have PascalCase filenames. I guess it's not that bad. What do you think?

satello · 2018-01-23T21:38:20Z

src/bootstrap/initializer.js

+    const { isWeb3Loaded } = this.state
+    const { accounts, children } = this.props
+
+    return renderIf(


satello · 2018-01-23T21:39:30Z

src/bootstrap/kleros.js


-export default new Kleros(web3.currentProvider, process.env.STORE_PROVIDER)
+export { eth }
+export default new Kleros(eth.currentProvider, process.env.STORE_PROVIDER)


satello · 2018-01-23T21:43:55Z

src/constants/errors.js

-export const WEB3_NOT_RESOLVED =
-  'The object `web3` was not found. Maybe try to install Metamask https://chrome.google.com/webstore/detail/metamask/nkbihfbeogaeaoehlefnkodbefgpgknn?hl=en'
+export const ETH_NO_ACCOUNTS =
+  'web3 accounts were not found. Maybe try installing MetaMask: https://chrome.google.com/webstore/detail/metamask/nkbihfbeogaeaoehlefnkodbefgpgknn?hl=en'


I think the wording should be something like web3 accounts not found. Do you have MetaMask installed? ...

satello · 2018-01-23T21:45:51Z

src/containers/balance/index.js

@@ -1,31 +1,71 @@
 import React, { PureComponent } from 'react'


what is this balance page for? is it just an example container?

Yeah, I needed an example container so I could write an integration test with the whole redux saga fetch/loading/{receive,failed} flow.

epiqueras · 2018-01-24T01:36:14Z

@satello
I changed the readme .env file to use placeholders.

I changed the ETH_NO_ACCOUNTS message.

I got rid of all the utils that are not being used. Do you think there is anything else I should remove?

I also made the app more "generic" by changing bootstrap/kleros.js to bootstrap/dapp-api.js.

n1c01a5 · 2018-01-24T04:25:00Z

src/utils/create-reducer.js

-      newState = { ...newState, ['loading' + resource]: false }
+    for (const actionTypePrefix of Object.keys(automaticActionPluginMap)) {
+      const l = actionTypePrefix.length
+      const a = action.type.slice(0, l)


the name var l or a it's not very explicit

n1c01a5 · 2018-01-24T04:29:24Z

@satello I disagree with the ETH balance component most of dapp are need this metric and I think it's good to have an concrete interaction with the blockchain

n1c01a5 · 2018-01-24T04:29:46Z

@epiqueras good work

epiqueras added 9 commits January 23, 2018 15:13

ci(linter): enforce component type checking through prop-types

9ed0897

fix(styles): stop importing .scss files and import .css files

1251a68

Add normalize.css.

feat(components): make requires-meta-mask a reusable component

81358de

refactor: normalize code style and make app more generic

6282dbb

feat: handle fetch and create errors and abstract common logic

7cbc6e3

feat(balance): add Identicon component using react-blockies

8b2dd1f

Closes #5

feat(wallet): integrate account fetching logic into sagas

68ad892

feat(reducers): enhance UX drawing inspiration from Apollo Client

f1601d0

test: one hundred percent coverage

7097a98

Closes #4

epiqueras requested review from n1c01a5 and satello January 23, 2018 20:51

satello reviewed Jan 23, 2018

View reviewed changes

epiqueras added 2 commits January 23, 2018 20:19

docs(readme): replace volatile env vars with placeholders

f1e0d09

feat(errors): change ETH_NO_ACCOUNTS message

b48088d

epiqueras added 2 commits January 23, 2018 20:37

refactor(utils): remove unused utils

f9b3619

refactor(bootstrap): make app more generic with kleros='>dapp-api'

6405642

n1c01a5 reviewed Jan 24, 2018

View reviewed changes

n1c01a5 approved these changes Jan 24, 2018

View reviewed changes

n1c01a5 merged commit 4385155 into master Jan 24, 2018

		@@ -1,31 +1,71 @@
		import React, { PureComponent } from 'react'

feat/enhancements #8

feat/enhancements #8

Uh oh!

Conversation

epiqueras commented Jan 23, 2018

Uh oh!

coveralls commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 12

💛 - Coveralls

Uh oh!

satello left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epiqueras commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1c01a5 Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1c01a5 commented Jan 24, 2018

Uh oh!

n1c01a5 commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jan 23, 2018 •

edited

Loading

epiqueras commented Jan 24, 2018 •

edited

Loading

n1c01a5 Jan 24, 2018 •

edited

Loading