-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adapt frontend app to the same style as other aragon apps #10
Conversation
Use babel. USe SafeMath for contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
.gitignore
Outdated
dist | ||
package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably shouldn't be part of the .gitignore, as users (of the cloned repo) might want to check it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, it seems that it's used for IPFS too, so I should remove then dist
too: https://github.com/aragon/aragon-cli/blob/master/src/commands/apm_cmds/publish.js#L102
I'll try to think of a better solution.
app/App.js
Outdated
return ( | ||
<AppContainer> | ||
<div> | ||
<ObservedCount observable={this.state.state$} /> | ||
<ObservedCount observable={this.state.observable} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be this.props.observable
app/main.js
Outdated
componentWillUnmount() { | ||
window.removeEventListener('message', this.handleWrapperMessage) | ||
} | ||
handleWrapperMessage = ({ data }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment here to explain that this is for a handshake between Aragon Core and the iframe, since iframes can lose messages that were sent before they were ready.
Address PR #10 comments. It needs aragon/aragon-cli#111
case 'Decrement': | ||
return { count: parseInt(state.count, 10) - parseInt(event.returnValues.step, 10) } | ||
return { count: await getValue() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could get rid of the switch, or at least join those 2 cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to keep the switch so it was obvious what the pattern would be for a more complicated app :).
Use babel.
Use SafeMath for contract.