Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AddressBook] UI issues #1823

Closed
10 tasks done
cryptoBeliever opened this issue Jan 13, 2022 · 10 comments · Fixed by #1847
Closed
10 tasks done

[AddressBook] UI issues #1823

cryptoBeliever opened this issue Jan 13, 2022 · 10 comments · Fixed by #1847
Assignees

Comments

@cryptoBeliever
Copy link
Contributor

cryptoBeliever commented Jan 13, 2022

List of UI issues (#1814):

If something was decided to look different than on Figma please just comment.

  1. Go to the address book and click "Add contact". Result: New contact pop-up shows and buttons "Add contact" and "Backup address book". I would expect that buttons will stay in their place.

Before you click "Add contact":

image

After "Add contact" clicked:

image

  1. Whitelist | Blacklist header. Tab selection purple underscore is wider than on Figma. Also looks like the underscore is higher on Figma.

Figma:
image

Wallet:
image

  1. Single contact item size/text fonts are bigger in the wallet than in Figma. This causes fewer items to fit the screen.

Figma:
image

Wallet:
image

  1. Contact name ("Symbol NFT Marketplace") is cut but there is plenty of space to show it (entire space should be used).

Figma:
image

Wallet:
image

  1. In the wallet there are horizontal lines between contact items (can be seen on screenshots above). In Figma, there are no such items.

  2. On Figma in blacklist contact address is on top and name on the bottom. As I understand this is a mistake on Figma? In the wallet, it's the same as for the whitelist (name top, address bottom).

  3. Signing partial transaction. View Transaction requires a signature:

  • There is another message "Transaction requires signature" vs "Transaction awaiting co-signature(s)". It's expected?
  • Button size is different than on Figma.
  • Accept button text on Figma is "Accept..." instead "Accept"
  • Left side of Reject button and text above should be aligned with transaction details left side

Figma:
image

Wallet:
image

  1. Similar issues as above with "Reject" view
  • Alignment, Text size, button size
  • I would change "New contact name" to "Contact name" or "Name"

Figma:
image

Wallet:
image

  1. Caution screen:
  • Missing image with "Caution" text
  • Again similar issues with alignment and size.
  • I think that Password controls (label, input, button) should show after the user accept the checkbox. What do you think @coiki ?
  • Password form is other than on Figma (on Figma on entire page). Is that expected?
  • When I click checkbox I have effect like this:
    image
  • on Figma, there is "I understand" in wallet "I understand and wish to proceed".

Figma:
image

Wallet:
image

  1. Add new address after signing partial:
  • We agreed to change text "Transaction signed and confirmed by network." to "Transaction successfully sent. Pending to be confirmed by the network."
  • Currently "Show in the explorer" links to Account but should Link to transaction

image

@bassemmagdy
Copy link
Contributor

Addressed all issues in PR #1814 except 6 (I believe it was a mistake on figma) and 9 as images with suitable sizes not ready yet.

@cryptoBeliever
Copy link
Contributor Author

  1. Fixed
  2. Looks better. Still underscore on Figma is a bit wider than the text above. If it's easy to fix, we could do that.
  3. Still looks like items are higher than on Figma. From Mosaic to Aggregate on Figma it fits 5 elements but after a change on desktop wallet only 4. Could you verify that? Maybe it's icon size or maybe a white break between elements.
  4. Fixed
  5. Fixed
  6. Ok. Assume its mistake on Figma.
  7. Fixed
  8. There is a missing question mark in the text.
  9. Ok. As I understand it will be fixed with images (still problem with checkbox when checked, alignment). @coiki what do you think about showing password input on checkbox selection.
  10. When pop-up with "New address detected" address is grey. Could you make it disabled but more visible?

@postoronnii
Copy link

postoronnii commented Jan 18, 2022

@cryptoBeliever @bassemmagdy maybe it makes sense to increase the size of letters? In my opinion they are too small and we have design mismatch with My Accounts. + we could show the full address, otherwise we have empty space in column

image

image

@postoronnii
Copy link

@cryptoBeliever @bassemmagdy Able to Export Blacklisted transactions despite that Show blacklisted transactions button was not selected.

@bassemmagdy
Copy link
Contributor

Able to Export Blacklisted transactions despite that Show blacklisted transactions button was not selected.

@NikoCyber Why do you expect them to be excluded in exported transactions?

maybe it makes sense to increase the size of letters? In my opinion they are too small and we have design mismatch with My Accounts. + we could show the full address, otherwise we have empty space in column.

I agree on the design mismatch part as in my accounts we have this with horizontal line separator and different dimensions:
Screen Shot 2022-01-19 at 4 02 16 AM
but in my contacts which should look alike AFAIU, we got this
Screen Shot 2022-01-19 at 4 02 20 AM
@cryptoBeliever @coiki If we want to proceed with that please reopen a new issue for aliging the other menus according to new design.

@bassemmagdy
Copy link
Contributor

  1. Fixed
  2. Looks better. Still underscore on Figma is a bit wider than the text above. If it's easy to fix, we could do that.
  3. Still looks like items are higher than on Figma. From Mosaic to Aggregate on Figma it fits 5 elements but after a change on desktop wallet only 4. Could you verify that? Maybe it's icon size or maybe a white break between elements.
  4. Fixed
  5. Fixed
  6. Ok. Assume its mistake on Figma.
  7. Fixed
  8. There is a missing question mark in the text.
  9. Ok. As I understand it will be fixed with images (still problem with checkbox when checked, alignment). @coiki what do you think about showing password input on checkbox selection.
  10. When pop-up with "New address detected" address is grey. Could you make it disabled but more visible?

@cryptoBeliever 2 is not as easy to control border's length, (3-8-10) should be fixed.

  • for (9) we can have that as a separate PR

@cryptoBeliever
Copy link
Contributor Author

cryptoBeliever commented Jan 19, 2022

we could show the full address, otherwise, we have empty space in column

@NikoCyber I agree that we should show the full address if we can present it (there is space)
@bassemmagdy @NikoCyber regarding how it looks compared to accounts view I think it's a question to @coiki and Biagio (I just commented difference between what is done and Figma screens).

Able to Export Blacklisted transactions despite that Show blacklisted transactions button was not selected.

@NikoCyber I think we should export all same as other filters that don't work on exported tx. I think users want to see all there as a account full history. From other side CSV export requires a lot of improvements.

@cryptoBeliever
Copy link
Contributor Author

cryptoBeliever commented Jan 26, 2022

Ad8. @bassemmagdy Please change "New contact name" to "Name
Ad9. @coiki please have a look if we want to change something here (maybe we should show password input when user click "I understand")?. In Figma, this screen looks different.

When the "I understand" checkbox is not checked:

image

When the "I understand" checkbox is checked:

image

@bassemmagdy checkbox looks bad (additional white part) when we "check" it:

image

  1. @coiki as I understand we should use "Block" instead "Blacklist" in wallet? Should we keep header whitelist/blacklist in address book?

I mean this view:

image

@cryptoBeliever
Copy link
Contributor Author

cryptoBeliever commented Feb 14, 2022

@OlegMakarenko what left (beside that you can review views):

  1. Caution screen:
  • missing image with "Caution" screen
  • I think Password controls (label, input, button) should show after the user accept the checkbox.
  • When I click the checkbox, I got strange artifact under it.
  • Clicking in "I understand" should check the checkbox too.
  1. New address detected screen:
  • We show address added in grey. It's not fully visible. Would be possible to show the entire address?
  1. We changed "Blacklist" to "Block". Not sure if we want to change it also in the address book.

@OlegMakarenko OlegMakarenko self-assigned this Feb 16, 2022
@OlegMakarenko OlegMakarenko linked a pull request Feb 16, 2022 that will close this issue
@cryptoBeliever
Copy link
Contributor Author

cryptoBeliever commented Feb 17, 2022

@OlegMakarenko great job! What I noticed:

  • There is still a missing image (above orange area) for the "Caution" screen. We don't have this image? Did you ask Bassem? We decided to not add this image.
  • Account -> My contacts. We have still a whitelist/blacklist (buttons and headers) there. You decided to keep it because it's more clear? In address book we kept whitelist/blacklist naming.
  • Account -> My contacts. If you click "Add contact" or "Import address book" buttons jump above. Fixed.
  • Home -> History. There is a skull filter there for blocked transactions. Alt text is "Show blacklisted transactions", should be "Show blocked transactions". Fixed.

yilmazbahadir pushed a commit that referenced this issue May 13, 2022
* feature: blacklist/whitelist contacts in address book

* increase testing coverage and linting

* update unit tests

* update package-lock

* fix console error

* add type for qrCodeCapturer

* check network type before adding new contact

* highlight malicious transactions button when clicked

* fix contact list ui

* Add new contact modals

* add translation

* update

* fix msig modification signing

* update ModalTransactionCosignature.vue

* fix ui issues in #1823

* fix checkbox margin

* update blacklist filter to show empty list when there is not blacklisted contacts

* fix filtering and update unit tests

* fix ui issues

* fix: address book (#1847)

* fix: address book list styles. feat: add option to blacklist new contact

* fix: rewrite ModalTransactionCosignature warning section, fix flow, fix checkbox styles, add escape option

* feat: add blocked contact transaction view, add unblock option. fix: 'jumping' form

* fix: display contact name in the tx signer field

* task: replace pretty addresses with a plain ones

* feat: add signer address to tx details

* feat: make address filed redirectable to explorer page

* feat: add address popover with explorer url

* task: require blacklist contact name

* fix: aggregate txs get random names from the address book, fix tx filter

* fix: styles of the new address detected modal

* fix: detected address input styles, new transaction incorrect address

* fix: the accounts selector jumping action buttons

* fix: contact address edition doesn't work

* fix: rename blacklist => block and lint

* fix: missing property error

* fix: the contact list jumping action buttons

* fix: rename show blacklisted transactions => show blocked transactions

* fix: edit contact with the same address

* feat: improve cosignature warning, show unknown signer address, add explorer link, add caution text

* fix: account balance widget address label size

* task: change address row explorer link popover layout and styles

* fix: remove unused commented style props

* fix: presentation for  multilevel multisig tree

* fix: show warning for blocked multisig address

* fix: remove unnecessary tx detail scroll to sign form

* task: change copy requires => awaiting

* task: add ModalTransactionCosignature component test

* task: update translations

* fix: translation

* task: cleanup test

* task: update copy

* fix: the transaction list action icons position

* fix: remove blacklisted contacts from the send form

* feat: add tad for blocked addresses, fix modal aligment, remove blacklist contact name input

* fix: lint

* task: update translations, add locale unit test

* feat: add the scam alert link

* fix: typo

Co-authored-by: Steven Liu <xian.f.liu@gmail.com>
Co-authored-by: OlegMakarenko <33131259+OlegMakarenko@users.noreply.github.com>
Co-authored-by: Oleg Makarenko <olegomm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants