Vault Management UI (round 3) #4652

jacogr · 2017-02-23T13:45:28Z

Extending the work from #4446 & #4631, finally Closes https://github.com/ethcore/parity/issues/3501

Adds the following functionality -

Selection of vaults on default account creation screen
Display of vault info on list summaries

Future work -

Currently the Vault attach on create is only available on the single account creation mechanisms

CreateAccount vault selection -

Vault information on account summary -

Depends on the following to be merged -

~~SelectionList component https://github.com/ethcore/parity/pull/4639~~
~~ModalBox component https://github.com/ethcore/parity/pull/4637~~
~~Hover info cards https://github.com/ethcore/parity/pull/4621~~
~~Previous round of Vault UI changes https://github.com/ethcore/parity/pull/4631~~

ngotchac · 2017-03-03T12:33:00Z

js/src/modals/CreateAccount/ChangeVault/changeVault.js

+@observer
+export default class ChangeVault extends Component {
+  static propTypes = {
+    store: PropTypes.object.isRequired,


It's a bit hard to know which store we're talking about here. Could use the same naming as vaultStore

Makes sense, should have renamed with the others.

On looking at the code. Store is the CreateAccount/store - used like that in all components in CreateAccount. (Not renaming it here accross all)

Well, it's obvious what store it is inside the CreateAccount context. It's less obvious in the ChangeVault context (you cannot just look at the imports)

~/ui/CreateAccount/ChangeVault

But it could be used elsewhere : views/Account/Header is not only used for Accounts but also for Address for example. Don't want to make a big thing, but I think that renaming it doesn't cost much and adds some readability.

No issues either way, but not going to do it just for the component and the others are out of scope for the PR. We should probably do it everywhere, we pass it everywhere as store. (Not just this component). Issues -

As you said, it may not be obvious from reading what we are dealing with (fails a bit in "write code for reading")

It conflicts with Redux store naming, i.e. when a MobX store & Redux is used (couple of places), the MobX store cannot be called 'store' in props

Should be logged for a global refactor/cleanup - https://github.com/ethcore/parity/issues/4748

ngotchac · 2017-03-03T12:36:34Z

js/src/ui/Form/VaultSelect/vaultSelect.js

+    isOpen: false
+  };
+
+  vaultStore = this.props.vaultStore || VaultStore.get(this.context.api);


Wouldn't it be better to choose whether it should be passed as prop or not? I find it a bit confusing

Actually only done this way for testing.

So.... ? One way or the other is fine, but we should choose one for consistency

It only gets use by the tests here.

So as a prop ? Thus we don't need the VaultStore.get(this.context.api) ?

The tests (e.g. vaultSelect.spec.js) passes thrhough as prop to tightly control the conditions. Normal operations is via getter.

Is there any other components that has this kind of logic ? I think that if we really want to have clean logic, we would have one pure component that gets everything via its props and that get tested, and one wrapper component that passes to this pure component the right props from whatever (a store instance, context, ...)

ngotchac · 2017-03-03T12:37:59Z

js/src/modals/CreateAccount/store.js

@@ -220,7 +226,28 @@ export default class Store {
    this.stage--;
  }

-  createAccount = () => {
+  createAccount = (vaultStore) => {


Wouldn't it make sense to attach the vaultStore to this Store before, instead of passing this as an argument (might be used for other methods)?

No. We specifically don't want it in FirstRun. So it is an option, not a requirement.

Could be optional without being an argument, no?

I prefer passing it here when we create, only used at this point and set by the caller as an option.

ngotchac · 2017-03-03T12:38:40Z

The Vaults are showing twice...

And in Edit Meta : Warning: Failed prop type: The prop valueis marked as required inVaultSelect, but its value is undefined.

jacogr · 2017-03-03T12:58:03Z

Last one is another botched merge. :(

ngotchac · 2017-03-03T12:59:45Z

Also, could we also use the tagged version of the Vault for the Account Summary in an Account page. It's not that great right now

jacogr · 2017-03-03T13:14:08Z

Not doing that in here. I will as soon as we have a proper tag mechanism in-place, as it stands it is just more duplication which I'm not up for.

ngotchac · 2017-03-03T14:09:41Z

Looks good, still some code-style gumbles, not blocking though.

jacogr added 30 commits February 20, 2017 11:23

Render Dapps via SectionList

cb9457a

Initial rendering of accounts via SectionList

7f27061

Width vars

2599ae4

Allow classNames in certifications & tags

806e8e9

Overlay of info on hover

6a00c50

Adjust hover balances

dbca85b

Merge branch 'master' into jg-sectionlists

e2e0d66

Large owner icons (align with vaults)

16c7ebb

Consistent block mined at message

c3c1b27

Attach ParityBackground to html

9aab39b

Adjust page padding to align

41b21ee

Lint fixes

be0b81e

Link to different types of addresses

c3167a0

Make content parts clickable only (a within a)

94d429f

Force Chrome hardware acceleration

5ff264c

Trust the vendors... don't go crazy with transform :)

69a4209

Use faster & default transitions

3ad9ec0

Add VaultMeta edit dialog

3a482ef

Updated (WIP)

955f1f8

Meta & password edit completed

1f9a98b

Added SelectionList component for selections

d20f23f

Use SelectionList in DappPermisions

c7e3491

AddDapps uses SelectionList

1e27a10

Fix AccountCard to consistent height

a5abb28

Merge branch 'master' into jg-vaults-2

c057003

Display type icons in creation dialog

9a66e48

Complimentary colours

db227e1

Merge branch 'master' into jg-selectionlist

f4b1c03

Convert Signer defaults to SelectionList

69105a9

Fix Geth import - actually pass addresses through

ebd3130

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 Mar 3, 2017

ngotchac reviewed Mar 3, 2017

View reviewed changes

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 Mar 3, 2017

jacogr added 5 commits March 3, 2017 14:01

ChangeVault location

2478fe8

Bothced merge, selector rendering twice

6d6c6d4

Value can be undefined (no vault)

8bf2b5a

Merge branch 'master' into jg-vaults-3

e069e2c

Close selector on Select bug

ce993e7

jacogr added 2 commits March 3, 2017 14:16

Fix toggle botched merge

6c9a5e8

Merge branch 'master' into jg-vaults-3

378123a

Update tests

bb8122c

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 Mar 3, 2017

Add Vault Tags to Account Header

b3c029f

ngotchac approved these changes Mar 3, 2017

View reviewed changes

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 Mar 3, 2017

gavofyork merged commit 1548201 into master Mar 3, 2017

gavofyork deleted the jg-vaults-3 branch March 3, 2017 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vault Management UI (round 3) #4652

Vault Management UI (round 3) #4652

jacogr commented Feb 23, 2017 •

edited

ngotchac Mar 3, 2017

jacogr Mar 3, 2017

jacogr Mar 3, 2017

ngotchac Mar 3, 2017

jacogr Mar 3, 2017

ngotchac Mar 3, 2017

jacogr Mar 3, 2017

ngotchac Mar 3, 2017

jacogr Mar 3, 2017

ngotchac Mar 3, 2017 •

edited

jacogr Mar 3, 2017

ngotchac Mar 3, 2017

jacogr Mar 3, 2017

ngotchac Mar 3, 2017

ngotchac Mar 3, 2017

jacogr Mar 3, 2017

ngotchac Mar 3, 2017

jacogr Mar 3, 2017

ngotchac commented Mar 3, 2017 •

edited

jacogr commented Mar 3, 2017

ngotchac commented Mar 3, 2017

jacogr commented Mar 3, 2017

ngotchac commented Mar 3, 2017 •

edited

Vault Management UI (round 3) #4652

Vault Management UI (round 3) #4652

Conversation

jacogr commented Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngotchac Mar 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngotchac commented Mar 3, 2017 • edited

jacogr commented Mar 3, 2017

ngotchac commented Mar 3, 2017

jacogr commented Mar 3, 2017

ngotchac commented Mar 3, 2017 • edited

jacogr commented Feb 23, 2017 •

edited

ngotchac Mar 3, 2017 •

edited

ngotchac commented Mar 3, 2017 •

edited

ngotchac commented Mar 3, 2017 •

edited