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

AccountCreate updates #3988

Merged
merged 58 commits into from
Jan 24, 2017
Merged

AccountCreate updates #3988

merged 58 commits into from
Jan 24, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 28, 2016 •
edited
Loading

  • Add store to CreateAccount
  • DRY up, removing duplication (single responsibility in store)
  • Clean up all update callbacks
  • Change FirstRun to use new creation
  • tests for components changed/added

PR is much bigger (in terms of LOC) than I'm typically comfortable with submitting, but at the same time it is just the single (very large) modal.

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Dec 28, 2016
@jacogr jacogr changed the title AccountCreation updates AccountCreate updates Dec 28, 2016
jacogr added 24 commits December 28, 2016 15:24
@jacogr
Update for renames
ecb30b4
@jacogr
Merge branch 'master' into jg-createacc-cleanup
1247ac9
@jacogr
Pass store around
ab2e1d7
@jacogr
createType into store
23122c0
@jacogr
Move stage into store
b9fc318
@jacogr
Update labels
8b5d029
@jacogr
Merge branch 'master' into jg-createacc-cleanup
88536d5
@jacogr
Define stages
26d921a
@jacogr
address into store
d7aac87
@jacogr
Add @observer
8374e8d
@jacogr
Retrieve name from store
c04c97c
@jacogr
Store phrase in store
18aa984
@jacogr
isWindowsPhrase into store
c3ea88c
@jacogr
Merge branch 'master' into jg-createacc-cleanup
3dd861a
@jacogr
gethAddresses to store
7fca950
@jacogr
Store manages geth addresses
ad4a195
@jacogr
passwordHint into store
3614b57
@jacogr
Merge branch 'master' into jg-createacc-cleanup
8051e62
@jacogr
Fix build
4abe154
@jacogr
rawKey into store
bf8a1b5
@jacogr
import json files
a013998
@jacogr
Merge branch 'master' into jg-createacc-cleanup
b80ed13
@jacogr
name set direct from component
d499a2b
@jacogr
No parent change callbacks
ca7788b
@jacogr jacogr removed the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jan 23, 2017
jacogr and others added 3 commits January 23, 2017 13:54
@jacogr
Merge branch 'master' into jg-createacc-cleanup
50e998d 
# Conflicts:
#	js/src/modals/CreateAccount/AccountDetailsGeth/accountDetailsGeth.js
#	js/src/modals/CreateAccount/NewGeth/newGeth.js
#	js/src/modals/CreateAccount/NewImport/newImport.js
@jacogr
New eslint update fixes
d429c72
@ngotchac
Merge branch 'master' into jg-createacc-cleanup
ae93f75
@ngotchac
Copy link
Contributor

Logs Warning: Failed prop type: Invalid prop 'owners' supplied to 'Summary'. as soon as I open the Create Account modal. Not sure what's going on.

When you choose something else than the default Create new account manually option and then go back, the default is selected and clicking Next brings you to the non-default option previously selected ; it's misleading.

Importing from Geth seems broken. The addresses are imported into my Address Book.

Looks good otherwise

@ngotchac ngotchac added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 23, 2017
@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jan 23, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Jan 23, 2017 •
edited
Loading

Current Geth import on master is not working. (It doesn't filter correctly, always yielding 0 accounts.) To try and replicate the issue here, fix that bug to test and it yields exactly the same Geth import outcome - it would seem that the Name is set before the account is actually imported, i.e. it never shows up under accounts. (Once deleted from the address book it does. Logged it since I don't believe it is related to new changes and does feel like a non-UI issue.)

The other 2 issues addressed - selection based on type in store, array or array-like object in summary (with copy operator in attached store)

@jacogr jacogr mentioned this pull request Jan 23, 2017
Closed
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 23, 2017
@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 24, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Jan 24, 2017 •
edited
Loading

@jacogr Not sure why setting owners: nullableProptype(arrayOrObjectProptype()) in src/views/Accounts/Summary/summary.js has anything to do with Account Creation ?

@ngotchac ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jan 24, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Jan 24, 2017 •
edited
Loading

@ngotchac We need MobX 3.0.0 for observable.shallowObject. (Which is the correct solution as per docs). Current version doesn't have that extension on the observer function. In addition, it will now clone objects instead of taking them as-is by default.

As-is the work-around does its job, upgrading removes the visible issue entirely for any other stores.

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 24, 2017
@jacogr jacogr mentioned this pull request Jan 24, 2017
Closed
@jacogr jacogr merged commit 0643303 into master Jan 24, 2017
@jacogr jacogr deleted the jg-createacc-cleanup branch January 24, 2017 15:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@derhuerst derhuerst derhuerst left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jacogr @ngotchac @derhuerst