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

new InputAddressSelect component #3071

Merged
merged 30 commits into from
Nov 3, 2016
Merged

new InputAddressSelect component #3071

merged 30 commits into from
Nov 3, 2016

Conversation

derhuerst
Copy link
Contributor

@derhuerst derhuerst commented Nov 1, 2016

This PR gives InputAddressSelect an overhaul. It fixes #3008. #3006 gets fixed indirectly because by default the input is empty.

The new idea behind the rewrite was to make it as comfortable as possible to fill in an address. There's completion by name & address and no toggle. You can enter an address both with and without a leading address.

What #3057 does for InputAddress, is done by this PR for InputAddressSelect.

Edit: I haven't checked for compatibility in all the places where InputAddressSelect is used. Will do that tomorrow.

@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. M6-ui labels Nov 1, 2016
@derhuerst
Copy link
Contributor Author

Let me know what you think about the default IdentityIcon being gray. (It will show the pattern once the input begins with 0x.)

@jacogr 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 Nov 1, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 1, 2016

Marked in-progress until you are comfortable and it actually has been checked. The risk is high, do a proper testing job on this one first before PR-ing.

@jacogr
Copy link
Contributor

jacogr commented Nov 1, 2016

As per @ngotchac PR - unless the address is valid, no IdentityIcon at all. It only gets displayed when the address is valid.

@chevdor
Copy link

chevdor commented Nov 1, 2016

Tested in the current version. I like the direction this is taking.

I miss the ability to click on the control to pull the list of known addresses and select from there.
While no absolute must, the previous resolution from a given address to a known name was nice and is (so far) missing in what I tested.

The toggle edit/search disappeared, which IMHO is a GREAT thing! Well done @derhuerst.
Warning, in the version I tested, the validation was gone: 0x12234 was not seen as invalid address, 0xasdf either.

I am currently working on an address component (not react), here are some screenshots if that can help:
screen shot 2016-11-02 at 00 31 21
screen shot 2016-11-02 at 00 31 33
screen shot 2016-11-02 at 00 30 55
screen shot 2016-11-02 at 00 30 27

@chevdor
Copy link

chevdor commented Nov 1, 2016

@jacogr in the current version, this is not the case:
screen shot 2016-11-02 at 00 34 53

@derhuerst
Copy link
Contributor Author

Warning, in the version I tested, the validation was gone: 0x12234 was not seen as invalid address, 0xasdf either.

Will address.

@gavofyork
Copy link
Contributor

See also #3088 and #3091.

@gavofyork
Copy link
Contributor

address both with and without a leading address

???

@derhuerst
Copy link
Contributor Author

derhuerst commented Nov 2, 2016

@gavofyork

address both with and without a leading address

???

typo. You can enter an address both with and without a leading 0x.

@chevdor
Copy link

chevdor commented Nov 2, 2016

See #3093

@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 2, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 2, 2016

Comments -

  • Step in the right direction, it shows promise
  • UI is now inconsistent, this field doesn't not have the placeholders or labels in the correct places
  • looking at 2 icon fields side-by-side - they don't match up

inconsistent-inputs

  • Own accounts always first, then addresses, then contracts (and sorted, when dealing with text, that is what people expect)

own-first

  • Invalid addresses still show IdentityIcons

invalid-error-identityicon

  • I'm assuming your comment from yesterday is not valid anymore? (re the testing)

@ngotchac
Copy link
Contributor

ngotchac commented Nov 2, 2016

I like better the icon outside of the input as @derhuerst did. I think it should be like that for every input with an icon.

@jacogr
Copy link
Contributor

jacogr commented Nov 2, 2016

I don't have a preference either way - but if it gets moved, it gets moved everywhere - needs to be consistent, cannot do it half-way. (nd do bear the copy in mind as well, we know there are some fields that needs to move to address that is copyable, so if outside - alignment and ll elements needs to be taken into account)

With the above, I think the best first-round option is to keep it where it was (no other impacts) and then look in another issue how to move everything out.)

@jacogr 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 Nov 2, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 2, 2016

Updated. @derhuerst was 100% correct in his assessment - instead of re-inventing the wheel, rather use the AddressSelect component as developed by @ngotchac and adapt it to also take care of "copy text" inputs. Basically the code between the 2 components has to be 90% the same - rather don't repeat the same code again. Also solves the styling issues, since already taken care of in the original AddressSelect component for consistency.

In short: Simplified greatly, re-used what is there. Video of it in action -

https://youtu.be/wOW18OtXTzo

Excuse the - (a) slugishness, Parity is syncing and (b) crash of Parity (stack-trace in #retreat)

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A0-pleasereview 🤓 Pull request needs code review. labels Nov 2, 2016
@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 Nov 2, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 2, 2016

Updated. Closes https://github.com/ethcore/parity/issues/3073 - contracts in the address search.

https://youtu.be/_ooiw0FysVk

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c8a7ebf on new-input-address-select into * on master*.

@GitCop
Copy link

GitCop commented Nov 3, 2016

There were the following issues with your Pull Request

  • Commit: ec2d3ec
    • Your commit message body contains a line that is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity


This message was auto-generated by https://gitcop.com

@GitCop
Copy link

GitCop commented Nov 3, 2016

There were the following issues with your Pull Request

  • Commit: ec2d3ec
    • Your commit message body contains a line that is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity


This message was auto-generated by https://gitcop.com

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 3, 2016
@gavofyork gavofyork merged commit 5ae737f into master Nov 3, 2016
@gavofyork gavofyork deleted the new-input-address-select branch November 3, 2016 10:57
@chevdor
Copy link

chevdor commented Nov 3, 2016

Should be used for #3134

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.

Address inputs in non edit mode should accept addresses
7 participants