Skip to content
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

Add the UI to view and buy tokens #6

Merged
merged 5 commits into from
Apr 19, 2018

Conversation

come-maiz
Copy link
Contributor

Requires #5

@frangio frangio self-requested a review April 18, 2018 20:26
class Wallet extends Component {

componentWillMount() {
Store.dispatch(AccountActions.findAccount()).then(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This same action is being dispatched in the ValidateMe component. I think we should only dispatch it once. Then, in Wallet for example, we should have a componentDidUpdate method where we check if this.props.address !== prevProps.address, and if so we dispatch getBalance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And the only time it should be dispatched should probably be in a component upper in the tree. I am reminded of react-web3 where there is a Web3Provider component wrapping the rest of the application which provides and updates the accounts. I would like us to try that library but we can do with a simpler thing for now.

@frangio
Copy link
Collaborator

frangio commented Apr 18, 2018

Are we planning to use mapDispatchToProps as was recommended in another review?

@come-maiz
Copy link
Contributor Author

I did mapDispatchToProps in #8.
I will try moving findAccount higher, and I reported #10 to experiment with react-web3.

@come-maiz
Copy link
Contributor Author

ok, I added Account.react.js, which is now the only one who gets the account. @frangio please take a look to see if it's good enough while we get react-web3.

Copy link
Collaborator

@frangio frangio left a comment

Choose a reason for hiding this comment

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

This is good. I think the Account component should be a parent of all the other components that use the address. This is how Web3Provider works, and I think this is in general the "Provider" pattern.

What we have now is good enough though.

Copy link
Collaborator

@frangio frangio left a comment

Choose a reason for hiding this comment

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

I haven't tested this on my machine yet but let's move forward.

@come-maiz
Copy link
Contributor Author

I thought about making it a parent. Just a little, not much. And it seemed to me that because we share the store everywhere, the hierarchy doesn't matter much. I will dig more when using the Web3Provider, but this makes me think that it's also not important where you put it: https://www.npmjs.com/package/react-web3#redux-support

Thanks again! I hope that getting less things to fix from you means that I'm getting the hang of this, and not that you are getting tired ;)

merging...

@come-maiz come-maiz merged commit 988b453 into TPL-protocol:master Apr 19, 2018
@come-maiz come-maiz deleted the feature/wallet branch April 19, 2018 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants