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

Use accountsInfo instead of eth_accounts for first check #3618

Merged
merged 1 commit into from
Nov 25, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Nov 25, 2016

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Nov 25, 2016
}

_checkAccounts () {
this._api.parity
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to make this call each time the app if started ? I guess if showFirstRun is set to 0 in LS, we shouldn't need to display this modal even if I deleted all my accounts.
Plus, if this.firstrunVisisble is true, this call is useless.
Lastly, can't we tweak the window to skip the account creation if the user already have some created accounts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually have the above actually logged as an enhancement, https://github.com/ethcore/parity/issues/3583. (Won't get in with 1.5 though)

Didn't try to re-invent the world, just tried to take what we have and change the call - already did more than needed with the move to a store to clean up a little bit. (With the eth_account changes it will show up each and every time, so this is needed before that change lands)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay fair.

@ngotchac ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 25, 2016
@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Nov 25, 2016
@gavofyork
Copy link
Contributor

no js build on the ci?

@gavofyork gavofyork merged commit a51066b into master Nov 25, 2016
@gavofyork gavofyork deleted the jg-list-accounts branch November 25, 2016 17:46
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.

None yet

3 participants