Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

Fix: Display abbreviated addresses consistently #1174

Merged
nicosampler merged 30 commits intodevelopmentfrom
issue-1144
Aug 11, 2020
Merged

Fix: Display abbreviated addresses consistently #1174
nicosampler merged 30 commits intodevelopmentfrom
issue-1144

Conversation

@nicosampler
Copy link
Contributor

Closes #1144.

@nicosampler nicosampler self-assigned this Jul 29, 2020
@mmv08
Copy link
Contributor

mmv08 commented Jul 30, 2020

Could you please include a meaningful PR name instead of issue number?

@nicosampler nicosampler changed the title Issue 1144 Fix: Display abbreviated addresses consistently Jul 30, 2020
@github-actions
Copy link

github-actions bot commented Jul 30, 2020

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Jul 30, 2020

Travis automatic deployment:
https://pr1174--safereact.review.gnosisdev.com/app

@nicosampler nicosampler marked this pull request as ready for review July 30, 2020 18:56
@lukasschor
Copy link
Contributor

image

alignment is a bit off

@lukasschor
Copy link
Contributor

image

Is it intentional that this PR does not yet cover these addresses? (Would be fine for me, but then we should just create a follow-on ticket)

@lukasschor
Copy link
Contributor

image

Same for these addresses in the creation transaction. Here I'm even more in favor of adding it later if it takes more than a couple of minutes change. :)

@lukasschor
Copy link
Contributor

image

Not so sure about the on-click outline. I like that there is more feedback that the address was copied to clipboard, but:

  1. the outline is a bit off (more space below)
  2. I think the outline should eventually still go away (e.g. after 2-3 seconds).

@lukasschor
Copy link
Contributor

image

Also slight alignment issues here. If that should be somehow complicated to fix for some reason, I'm also happy to deactivate my OCD here. :P

@mmv08
Copy link
Contributor

mmv08 commented Jul 31, 2020

I think the outline should eventually still go away (e.g. after 2-3 seconds).

unfortunately that's how browsers work, it will go away if you click somewhere else
Also fyi why this is needed: http://www.outlinenone.com/

@ghost
Copy link

ghost commented Jul 31, 2020

Travis automatic deployment:
https://pr1174--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Jul 31, 2020

Travis automatic deployment:
https://pr1174--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Aug 3, 2020

Travis automatic deployment:
https://pr1174--safereact.review.gnosisdev.com/app

@francovenica
Copy link
Contributor

francovenica commented Aug 3, 2020

EDIT: this issue is in DEV as well, and it just with that safe address. Still I need that safe and it could be in others safe we gotta fix it. I'll create a separated issue

@nicosampler
I cannot load safes in this PR. I get this error in the console
This is the safe I'm trying to load: 0x9913B9180C20C6b0F21B6480c84422F6ebc4B808
I tried in Chrome and Brave

image.png

@lukasschor lukasschor self-requested a review August 4, 2020 13:38
Copy link
Contributor

@lukasschor lukasschor left a comment

Choose a reason for hiding this comment

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

Product-wise it looks good!

@lukasschor
Copy link
Contributor

For some reason all tx hashes lead to an invalid address page on etherscan. Such as this one: https://rinkeby.etherscan.io/address/0xb052804c5a723c5cfc33a754099209aa119408f7a2a96c4df6bd75dda561a866

@nicosampler
Copy link
Contributor Author

Good catch @lukasschor. I pushed the fix, but it won't work in the review app until this PR 5afe/safe-react-components#50 is merged.

@nicosampler
Copy link
Contributor Author

CC: @mikheevm

@mmv08
Copy link
Contributor

mmv08 commented Aug 5, 2020

@francovenica @lukasschor I think it's important to show the full address at least somewhere. I've seen a post where the user mistyped one symbol of an address and transferred 10000$ to the wrong address

@lukasschor
Copy link
Contributor

@francovenica @lukasschor I think it's important to show the full address at least somewhere. I've seen a post where the user mistyped one symbol of an address and transferred 10000$ to the wrong address

Sure, this ticket is not about abbreviating all addresses, just about formatting the already abbreviated ones differently / consistently. Maybe at some point we could abbreviate them all and only show full address on hover.

@github-actions
Copy link

github-actions bot commented Aug 6, 2020

CLA Assistant Lite All Contributors have signed the CLA.

@nicosampler
Copy link
Contributor Author

@francovenica ready to test.
@lukasschor I fixed the problem with Etherscan links.

@ghost
Copy link

ghost commented Aug 7, 2020

Travis automatic deployment:
https://pr1174--safereact.review.gnosisdev.com/app

1 similar comment
@ghost
Copy link

ghost commented Aug 7, 2020

Travis automatic deployment:
https://pr1174--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Aug 11, 2020

Travis automatic deployment:
https://pr1174--safereact.review.gnosisdev.com/app

@dasanra
Copy link
Collaborator

dasanra commented Aug 11, 2020

This PR looks good to me and issues commented are solved.

This should be ready to merge to development

@nicosampler nicosampler merged commit f119813 into development Aug 11, 2020
@nicosampler nicosampler deleted the issue-1144 branch August 11, 2020 17:58
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2020
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.

Display abbreviated addresses as 0x1234…AbCd

5 participants