Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

Resolve contacts in send form & optimizations #524

Merged
merged 5 commits into from
Jan 31, 2018

Conversation

Nasicus
Copy link

@Nasicus Nasicus commented Jan 15, 2018

This does the following:

  • On type in address field now also works for wallet labels (did only work for contacts before)
  • Form cannot be sent if address is empty
  • Known addresses (contacts & wallet labels) are now always resolved and displayed below the field (it always bothered me, that you selected a contact, but didn't know which one it was after you did so, because only the address was displayed). I'm not yet sure if it's optimal from a UX standpoint:
    capture
  • Contacts & wallets can now be resolved without explicitly clicking on it in the dropdown (this only works if you type it 100% correctly, also it only works if the contact is non-ambiguous). Also while you're still typing the Invalid Address error is not shown if you're typing the name of a contact.
    resolve
    • If the contact is ambiguous for some reason (case insensitive!) an error is shown:
      ambiguous

Fixes: #520

@Nasicus
Copy link
Author

Nasicus commented Jan 16, 2018

Also added this info to the validation page, but kept it more subtile and decided to only show it as a icon with a tooltip (I also had it as a separate row once, but find it this way less disruptive):
tooltip

Also changed the labels from Contact / wallet to Contact or Account.

@Nasicus Nasicus force-pushed the feat/contact-not-valid branch 2 times, most recently from dae97b4 to 2f434c6 Compare January 17, 2018 13:49
@j-a-m-l j-a-m-l self-requested a review January 25, 2018 20:03
@Nasicus
Copy link
Author

Nasicus commented Jan 25, 2018

@j-a-m-l fixed conflicts ;)

@Nasicus
Copy link
Author

Nasicus commented Jan 26, 2018

I changed this a little. I now like it much more than what I did before, see screenshots here:
#537

@Nasicus Nasicus force-pushed the feat/contact-not-valid branch 3 times, most recently from 2fef211 to 1d41f48 Compare January 26, 2018 11:50
@j-a-m-l
Copy link
Contributor

j-a-m-l commented Jan 30, 2018

Now it works slightly different, please, @Nasicus take a look. Probably your way is better.

@Nasicus
Copy link
Author

Nasicus commented Jan 30, 2018

  • rebased & squashed everything into a single commit
  • integrated @j-a-m-l changes (looks MUCH better now than before - thanks for the "input"!)
  • icon is now displayed instead of the qr-code icon, if the entered value equals either a contact or an account (I think it's still user friendly, because it's very unlikely that you want to scan a QR code once you select a contact, if you still want to do that, you just have delete the input)

The final gif of how it looks and works:

resolve

- find account labels on type
- add icons for contacts & wallets
- always order by contacts, accounts, alphabetical
- resolve contacts on field blur if the match an existing account or contact exactly
- show account type info on validation page (contact / account)
- don't allow to send form when address is empty
Copy link
Contributor

@j-a-m-l j-a-m-l left a comment

Choose a reason for hiding this comment

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

I like how it's working now, but I'd change the account icons to be the same that are being used in the sidebar (client/app/src/components/account/templates/main-sidenav.html).

@Nasicus
Copy link
Author

Nasicus commented Jan 30, 2018

Problem is that they look almost the same as contacts.
I'd rather replace all account icons over the app with the wallet icon? (However there are 2 very similar "account" icons used right now, for wallet there is only one type)

@j-a-m-l
Copy link
Contributor

j-a-m-l commented Jan 30, 2018

There are 4 types:

  • Ledger
  • Account that are on the blockchain (at least 1 transaction) => maybe this one could be replaced by the account wallet balance icon.
  • Account that aren't (cloud off)
  • Delegates

I think that it's useful knowing which accounts haven't been used yet.

One way to differentiate them could be using different colours.

@Nasicus
Copy link
Author

Nasicus commented Jan 30, 2018

Damn it you're right - totally forgot about these.

However I don't think it would be good to use all these different icons in the search box, because the aim of the current two icons is to clearly distinguish between contacts and accounts / wallets.

What is your current proposed solution? I must send I don't know what'd be best...

@j-a-m-l
Copy link
Contributor

j-a-m-l commented Jan 31, 2018

I understand you point, but I think that it's better to maintain the familiar icons instead of using a new one.
Apart from that, it would be faster to spot a specific account thanks to the icon.

@Nasicus
Copy link
Author

Nasicus commented Jan 31, 2018

At @j-a-m-l what do you say now:

image

I also added a tooltip if you hover over the icon in the dropdown (of course the tooltip is also shown once you select an account/contact):
image

@Nasicus
Copy link
Author

Nasicus commented Jan 31, 2018

Also renamed Unique to Single (as discussed), see 34a21f7

Edit:
Now (758933a) I set the icons directly when myAccounts are returned - do you think this is better than calling a separate method each time we need it?

Copy link
Contributor

@j-a-m-l j-a-m-l left a comment

Choose a reason for hiding this comment

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

I think that setting the icon in myAccounts is enough (as long as the accounts don't change).

@j-a-m-l j-a-m-l merged commit fb07196 into ArkEcosystem:master Jan 31, 2018
@j-a-m-l
Copy link
Contributor

j-a-m-l commented Jan 31, 2018

Good work!

@Nasicus Nasicus deleted the feat/contact-not-valid branch February 1, 2018 09:04
alexbarnsley added a commit that referenced this pull request Dec 3, 2018
* fix: reload session after reset on app launch

* refactor: set i18n locale on session change

* fix: reload profile when leaving new profile page
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants