New Address Selector Component #3829

ngotchac · 2016-12-12T21:50:50Z

Add a new Address Selector component. This is a full width component that should make searching through addresses easier and nicer.

It is a first try, and I'm sure many things can be improved.
The InputAddressSelect Component is by default using this new component, but the old one is still available by just specifying it from props (we might want to remove this).

Included features:

Keyboard navigation: navigate through addresses with tab or left/right/up/down arrows
Select addresses without selecting the account
Copy the address when focusing on an account
Pressing enter when there's only one item selects it
More features...

jacogr · 2016-12-13T09:29:25Z

Marking WIP for the changes as discussed.

gavofyork · 2016-12-17T14:46:09Z

maybe get #3871 into this, too?

jacogr · 2016-12-22T09:02:36Z

Makes is very confusing when the contracts page swaps to the middle - initially I thought "these aren't contacts" -

Columns should stay in-place, not jump around in the UI

jacogr · 2016-12-22T09:03:28Z

js/src/ui/Form/AddressSelect/addressSelect.css

+  flex: 1;
+
+  .input {
+    font-size: 36px;


Could we make it 1.5em (my mistake, the headings are 1.5em, not 36px - don't know where I got that number from)

derhuerst · 2016-12-22T11:29:30Z

aspects i really like:

you can tab into the popup and it clearly shows you which item is focused. ❤️
it looks nice and structured!
the addresses are displayed.

[personal opinion] UX grumbles:

I'd like the popup to be less translucent. It's pretty noisy.
The address input you click in the beginning should have cursor: text;.
The close button should have a subtle, but noticeable hover effect, e.g. opacity.
On my screen, tags of accounts are almost invisible. Imagine this on a projector or in direct sunlight. Would prefer to have the accounts/contacts/contracts boxes dark, like in the rest of the UI.
Instead of "No account matches this query…", I'd use sth like "Proceed with 0x…". It sounds less like an error, as it's a perfectly valid step to proceed with a (still) unknown address.
Putting a custom/unknown address should still validate if it has the correct length. It may allow you proceed with 0x123, but should make it red.
The addresses should be shown as font-family: monospace;.
On narrow screens, the account names overflow the tags.
The "recipient address" input on top doesn't look aligned with the rest.

derhuerst · 2016-12-22T13:52:19Z

js/src/ui/AccountCard/accountCard.js

+          const range = document.createRange();
+          range.selectNode(element);
+          window.getSelection().addRange(range);
+          document.execCommand('copy');


You may want to consider all the quirks involved with the clipboard. Haven't found a focused & well-written module yet though.

derhuerst · 2016-12-22T13:58:48Z

js/src/ui/Form/AddressSelect/addressSelect.js

+      <InputAddress
+        accountsInfo={ accountsInfo }
+        allowCopy={ false }
+        disabled


General comment. readOnly would be appropriate here, but is not yet supported on <Input>. disabled means "the form control is not available for interaction".

derhuerst · 2016-12-22T14:01:08Z

js/src/ui/Form/AddressSelect/addressSelect.js

    }

-    return this.items[address];
-  }
+    const id = 'addressSelect_' + Math.round(Math.random() * 100).toString();


Would be nice to have a counter for this, renamed to sth like newId.

let lastId = 0 const newId = () => `id${++lastId}`

ngotchac · 2016-12-22T16:06:56Z

I've taken into account most of your comment @jacogr and @derhuerst

The only quirk left is are the tags merging into the name. This behavior also happens with other Accounts Cards (in Accounts or Contracts, etc.). At least it's a consistent quirk !
I'm not sure what the behavior should be : should some tags be hidden if the name overlays over them ? Should all the tags be hidden with a small icon/popover to show them on hover ?

jacogr · 2016-12-22T16:44:08Z

@ngotchac As you said, it is consistent behaviour, would not worry about it too much atm, as ugly as it is. (Once we have moved accounts over to use the same Card, we can re-visit.)

jacogr · 2016-12-22T16:52:12Z

Ok, not sure why we moved to monospaced fonts here for the address.

With this we are now wildly inconsistent since there is nowhere in the UI where we actually display addresses with monospaced fonts - not on any of the views, not in any of the drop downs, not in any of the inputs.

Apart from that, looks to be there.

derhuerst · 2016-12-22T16:56:59Z

Ok, not sure why we moved to monospaced fonts here for the address.

Guess @ngotchac followed my suggestion.

With this we are now wildly inconsistent since there is nowhere in the UI where we actually display addresses with monospaced fonts […].

You have a better overview over the UI. But I'm very sure it used to be like this a short time ago.

I'm completely fine with abbreviating the address (using <abbr>) as we have the copy button. But as as long, as we consider the address something important, worth the full length, I'd like it to be properly displayed (which imho means monospaced).

As a side note, while we're talking about the addresses: I'd like them all be either lower/upper case, not a mixture. Or is there a reason behind this?

coveralls · 2016-12-22T17:16:20Z

Coverage decreased (-0.04%) to 85.722% when pulling f814aea on ng-enhanced-address-selector into b444d23 on master.

jacogr · 2016-12-22T17:43:36Z

Mixed case is due to us following EIP55 for checksums

# Conflicts: # js/src/index.js

coveralls · 2016-12-23T16:14:38Z

Coverage decreased (-0.01%) to 85.524% when pulling ddc04d3 on ng-enhanced-address-selector into 74efb22 on master.

coveralls · 2016-12-24T16:15:58Z

Changes Unknown when pulling 61c5b9c on ng-enhanced-address-selector into ** on master**.

coveralls · 2016-12-24T16:25:25Z

Changes Unknown when pulling 61c5b9c on ng-enhanced-address-selector into ** on master**.

ngotchac added 12 commits December 12, 2016 02:09

WIP new address selector

ebc9844

Merge branch 'master' into ng-enhanced-address-selector

c91cf71

WIP Working New Account Selector

2c0599a

WIP Fully working Addres Selector (missing keyboards navigation)

bd4259b

WIP Address Selector

94b586c

Merge branch 'master' into ng-enhanced-address-selector

02a3248

Fully functionnal new Address Selector!

4cb5e42

Implement disabled prop

0297e94

Don't auto-open on focus + Text Selection fix

0ceec7b

Add copy address capabilities

532aca8

Better Address Selector Focus

54f0dad

Search from tags

250c51d

ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 12, 2016

jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 13, 2016

ngotchac added 11 commits December 14, 2016 19:13

[Address Selector] Better Focus // Parity Background

f8285d3

Linting

b10f5a0

[Adress Selector] Better focused input style

ab8542c

Better focus support for inputs

cc73b59

Fix style issue

ae69596

Add tags to accounts

0421106

linting

3a5c7dd

Add label to address selector

d0f505a

Removed old address selector + improved styling

388aee2

Fixing address selection issues

9568454

Merge branch 'master' into ng-enhanced-address-selector

a4a2d66

Merge branch 'master' into ng-enhanced-address-selector

8614705

jacogr reviewed Dec 22, 2016

View reviewed changes

derhuerst reviewed Dec 22, 2016

View reviewed changes

ngotchac added 2 commits December 22, 2016 16:33

Merge branch 'master' into ng-enhanced-address-selector

666b2e6

PR Grumbles Part 4

f814aea

jacogr added 5 commits December 23, 2016 14:59

Align font-family with rest of UI

9da2109

Merge branch 'master' into ng-enhanced-address-selector

290dfc1

# Conflicts: # js/src/index.js

Merge branch 'master' into ng-enhanced-address-selector

324a06e

Merge branch 'master' into ng-enhanced-address-selector

b81c8b6

Fix null value input

ddc04d3

jacogr added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 23, 2016

ngotchac added 2 commits December 24, 2016 16:20

Merge branch 'master' into ng-enhanced-address-selector

a3f33d9

Fix Webpack build...

61c5b9c

ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Dec 24, 2016

gavofyork merged commit 1ffc6ac into master Dec 27, 2016

gavofyork deleted the ng-enhanced-address-selector branch December 27, 2016 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Address Selector Component #3829

New Address Selector Component #3829

ngotchac commented Dec 12, 2016

jacogr commented Dec 13, 2016

gavofyork commented Dec 17, 2016

jacogr commented Dec 22, 2016

jacogr Dec 22, 2016 •

edited

derhuerst commented Dec 22, 2016

derhuerst Dec 22, 2016

derhuerst Dec 22, 2016

derhuerst Dec 22, 2016

ngotchac commented Dec 22, 2016

jacogr commented Dec 22, 2016

jacogr commented Dec 22, 2016

derhuerst commented Dec 22, 2016 •

edited

coveralls commented Dec 22, 2016

jacogr commented Dec 22, 2016

coveralls commented Dec 23, 2016

coveralls commented Dec 24, 2016

coveralls commented Dec 24, 2016

New Address Selector Component #3829

New Address Selector Component #3829

Conversation

ngotchac commented Dec 12, 2016

jacogr commented Dec 13, 2016

gavofyork commented Dec 17, 2016

jacogr commented Dec 22, 2016

jacogr Dec 22, 2016 • edited

Choose a reason for hiding this comment

derhuerst commented Dec 22, 2016

derhuerst Dec 22, 2016

Choose a reason for hiding this comment

derhuerst Dec 22, 2016

Choose a reason for hiding this comment

derhuerst Dec 22, 2016

Choose a reason for hiding this comment

ngotchac commented Dec 22, 2016

jacogr commented Dec 22, 2016

jacogr commented Dec 22, 2016

derhuerst commented Dec 22, 2016 • edited

coveralls commented Dec 22, 2016

jacogr commented Dec 22, 2016

coveralls commented Dec 23, 2016

coveralls commented Dec 24, 2016

coveralls commented Dec 24, 2016

jacogr Dec 22, 2016 •

edited

derhuerst commented Dec 22, 2016 •

edited