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

Vault Management UI (round 2) #4631

Merged
merged 36 commits into from
Feb 24, 2017
Merged

Vault Management UI (round 2) #4631

merged 36 commits into from
Feb 24, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Feb 22, 2017 •
edited
Loading

Extending the work from https://github.com/ethcore/parity/pull/4446, working towards completing https://github.com/ethcore/parity/issues/3501

Includes & requires (there should be merged first, relevant parts in here) -

Future work -

Edit vault -

  • Meta data edit (description & passwordHint)
  • Password edit

parity 2017-02-22 11-52-22

Account Vault info -

  • Show vault information when attached

parity 2017-02-23 09-38-22

Edit Meta -

  • Show current associated vault
  • Allow editing of vault (based on which are open)

parity 2017-02-23 11-38-17
parity 2017-02-23 11-38-44

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Feb 22, 2017
@jacogr jacogr mentioned this pull request Feb 22, 2017
Merged
@jacogr jacogr mentioned this pull request Feb 22, 2017
Merged
@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 Feb 23, 2017
@jacogr jacogr changed the title Vault management UI extended Vault Management UI (round 2) Feb 23, 2017
@jacogr jacogr mentioned this pull request Feb 23, 2017
Merged
@jacogr jacogr removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Feb 24, 2017
ngotchac
ngotchac suggested changes Feb 24, 2017
View reviewed changes
Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

Some small comments.

One issue: when editing a Vault meta data, the second new password input doesn't have to have the same value as the first one.

One question (outside of this PR): is there a plan on being able to delete a Vault?

js/src/modals/EditMeta/editMeta.js
vaultStore = VaultStore.get(this.context.api);

componentWillMount () {
this.vaultStore.loadVaults();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't it be loaded only once (the Vault Store) when the app starts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason it is done here is to ensure we have the current info. It is loaded already at this point, basically a refresh if/when it was changed outside of the window. (It should be ok, but rather safe than sorry)

js/src/modals/EditMeta/editMeta.js Outdated
<InputAddress
allowCopy={ false }
allowInvalid
disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

readOnly?

js/src/modals/VaultMeta/vaultMeta.js
class VaultMeta extends Component {
static propTypes = {
newError: PropTypes.func.isRequired,
vaultStore: PropTypes.object.isRequired
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the VaultStore passed as a prop here, and not in the EditMeta component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically not needed until it is needed, so don't want to pull it in in the parent.

Even where the info is displayed on the Account Header, this is still the case - all the required info is already available as part of meta to render the icon.

In the case of VaultMeta it gets called from ~/views/Vaults so already available, no need to create.

Copy link
Contributor

Choose a reason for hiding this comment

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

But isn't it a Singleton? The instance is saved in memory, so there's no need to re-create it, right? Just seems unbalanced to have the two approaches. No big deal though

js/src/modals/VaultMeta/vaultMeta.js
defaultMessage='close'
/>
}
onClick={ this.onClose }
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this has been discussed earlier, but I think that at least for new modals using the Portal, we shouldn't add this useless Close button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point it is done for the sake of consistency :(

js/src/modals/VaultSelector/vaultSelector.js
onClose: PropTypes.func.isRequired,
onSelect: PropTypes.func.isRequired,
selected: PropTypes.string,
vaultStore: PropTypes.object.isRequired
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above regarding the vaultStore as a prop

js/src/views/Account/Header/header.js

return (
<div className={ styles.vault }>
<IdentityIcon
Copy link
Contributor

Choose a reason for hiding this comment

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

Matter of personal taste here, but I find the Identity Icon a bit big here

@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 Feb 24, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 24, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 24, 2017

Fixed the password bug.

As for your question - would like to see both rename & delete.

@ngotchac
Copy link
Contributor

ngotchac commented Feb 24, 2017 •
edited
Loading

Have some errors in the console when modifying the password (in VaultMeta):

Failed prop type: Invalid prop `disabled` of type `object` supplied to `Button`, expected `boolean`.

Warning: Failed prop type: Invalid prop `disabled` of type `object` supplied to `EnhancedButton`, expected `boolean`.

@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 Feb 24, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 24, 2017
@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 Feb 24, 2017
@ngotchac
Copy link
Contributor

Last comments:

  • Find the Identity Icon in the Account Summary a bit big
  • Not sure about the Vault Store being passed as props sometimes, and sometimes not

@jacogr jacogr merged commit 570e6f3 into master Feb 24, 2017
@jacogr jacogr deleted the jg-vaults-2 branch February 24, 2017 17:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ngotchac ngotchac ngotchac approved these changes

Assignees
No one assigned
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Milestone
No milestone
Development

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