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

Small UI fixes #3966

Merged
merged 8 commits into from Dec 27, 2016
Merged

Small UI fixes #3966

merged 8 commits into from Dec 27, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 24, 2016 •
edited

  • Actually retrieve saved dapp visibility from storage (previously refreshing the page lost the visible apps, storage never taken into account)
  • Align SVG address icons on MethodDecoding (previous these were displayed too low for null & contract addresses - also Closes https://github.com/ethcore/parity/issues/3954)
  • Improve isNullAddress check (actual full value)

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 24, 2016
@ngotchac
Copy link
Contributor

ngotchac commented Dec 24, 2016 •
edited

Using null for every address in InputAddress is misleading. First, it breaks adding a new address to the address book. It also will result in a wrong information for transaction to the Zero Account for example. Shouldn't it be limited to informations in contracts ?

@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 Dec 24, 2016
@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 Dec 25, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Dec 25, 2016 •
edited

Made isNullAddress explicit & more robust (right PR to add it in, missed that bug in my checking)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 85.422% when pulling 9076d81 on jg-fixes into 9cfc72c on master.

@ngotchac
Copy link
Contributor

The only grumble is that in the Add Address Modal, when you enter 0x000000000000000000000000000000000000000 then another 0, the input is changed to null, which is a bit misleading : you cannot add the Zero Account to your address book.

@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 Dec 27, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Dec 27, 2016

Can add it, it just displays 'null' as the name. Made it so it doesn't display the name on input, only when queried.

@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 Dec 27, 2016
ngotchac
ngotchac suggested changes Dec 27, 2016
View reviewed changes
js/src/ui/Form/InputAddress/inputAddress.js
@@ -69,7 +69,7 @@ class InputAddress extends Component {
classes.push(!icon ? styles.inputEmpty : styles.input);

const containerClasses = [ styles.container ];
const nullName = value && new BigNumber(value).eq(0) ? 'null' : null;
const nullName = readOnly && isNullAddress(value) ? 'null' : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to check disabled as well (it's the one used in contracts)

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 27, 2016
@jacogr jacogr merged commit 19c8e55 into master Dec 27, 2016
@jacogr jacogr deleted the jg-fixes branch December 27, 2016 11:40
@jacogr jacogr mentioned this pull request Jan 3, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ngotchac ngotchac ngotchac requested changes

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.

None yet
3 participants
@jacogr @ngotchac @coveralls