Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Account view updates #4008

Merged
merged 9 commits into from
Jan 5, 2017
Merged

Account view updates #4008

merged 9 commits into from
Jan 5, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Jan 2, 2017

Not critical for 1.5, could wait if need be.

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Jan 2, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Jan 2, 2017
<Certifications
account={ account.address }
/>
<Certifications address={ account.address } />
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use address there, right?

this.setAddress(props.address);
this.setTest(props.isTest);

// TODO: When tracing is enabled again, adjust to actually set
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


this.setLoading(true);

// TODO: When supporting other chains (eg. ETC). call to be made to other endpoints
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we already use the chain ID ? Pass it to the Etherscan API call, which right now would only switch from main to test networks, but could potentially make the right transaction fetching call.
Like this we would use the chain ID instead of isTest

Copy link
Contributor Author

@jacogr jacogr Jan 4, 2017

Choose a reason for hiding this comment

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

Agreed. Didn't want to do it here since it may be one of those exploding tasks that turns into changes in 50+ files - however would like to start using netChain instead of isTest as soon as possible.

@ngotchac ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jan 4, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Jan 4, 2017

Tiny comments, but looks good overall. Cannot merge though

@ngotchac ngotchac 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 Jan 5, 2017
@jacogr jacogr merged commit 602a442 into master Jan 5, 2017
@jacogr jacogr deleted the jg-account-updates branch January 5, 2017 11:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants