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

Add support for wallets without getOwner() interface #3779

Merged
merged 8 commits into from
Dec 10, 2016
Merged

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Dec 9, 2016

This adds Mist Wallet support (which doesn't have the getOwner method).

Plus small fixes here and there (React errors from same keys for elements, stop fetching addresses from the registry twice if it is loading...)

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 9, 2016
@@ -89,7 +97,7 @@ export default class Registry {
return instance.getAddress.call({}, [sha3, 'A']);
})
.then((address) => {
console.log('lookupAddress', name, sha3, address);
console.log('[lookupAddress]', `(${sha3.slice(0, 5)}...${sha3.slice(-3)}) ${name}: ${address}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually revert this (at least the slicing), this does get used in management via contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -112,31 +112,36 @@ export default class Input extends Component {
{ this.renderCopyButton() }
<TextField
autoComplete='off'
name={ NAME_ID }
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are re-formatting this, would really like it in alphabetical order so there is no confusion about what goes where. (Always make it alphabetical regardless of type or function, no mental overhead - as it is it makes sense to somebody, with your changes that somebody is you, keep it generic.)

@@ -188,7 +188,7 @@ class TabBar extends Component {
return (
<ToolbarGroup>
<div className={ styles.logo }>
<img src={ imagesEthcoreBlock } />
<img src={ imagesEthcoreBlock } height={ 28 } />
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this is not in the css?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only because the CSS is loaded in JS, so there is a flash of ugly unstyled content on load. The logo was the thing that upset me the most.
With a bit more work, we should ship CSS files in production env anyway. This is just a quicker fix

<Container title='events'>
<div>
<Loading
size={ 2 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Single line is preferable here since it is only a single attribute.

isTest={ isTest }
events={ allEvents } />

<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the extra divs needed when the component itself is a div?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because the component has a background color set (black) and padding, which makes two components in a row merge without margins. Could replace padding with margin though, but need to make sure it doesn't break anything

@jacogr jacogr changed the title Add Mist Wallet support Add support for wallets without getOwner() interface Dec 9, 2016
@jacogr jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 9, 2016
@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 9, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 86.087% when pulling ec50a45 on ng-wallet-fixes into 2226324 on master.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 9, 2016
@jacogr jacogr merged commit 6655e7e into master Dec 10, 2016
@jacogr jacogr deleted the ng-wallet-fixes branch December 10, 2016 00:26
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