Skip to content
This repository was archived by the owner on Apr 4, 2022. It is now read-only.

[Explorer] Add Github + Etherscan logos to external links #1042

Merged
merged 10 commits into from
Feb 16, 2022
Merged

[Explorer] Add Github + Etherscan logos to external links #1042

merged 10 commits into from
Feb 16, 2022

Conversation

matextrem
Copy link
Contributor

Summary

Closes #1021

  • Added Github logo to Contracts link in footer.
  • Added Explorer link to external links in app (excluding tokens).

Screen Shot 2022-02-09 at 17 58 43
Screen Shot 2022-02-09 at 17 57 53
Screen Shot 2022-02-09 at 17 57 22

To Test

  1. Open any page. i.e: Home
    • You'll see github and explorer logos next to the links in footer.
  2. Open address page. i.e: /address/0xb6bad41ae76a11d10f7b0e664c5007b908bc77c9
    • You'll see explorer logo next to the top link.
  3. Open tx page. i.e: /rinkeby/tx/0x7b6a38411fa9c3cdcd9a9c1fde2d61041300c5567a3d0bbd8755dff29270e5b0
    • You'll see explorer logo next to the top link.

@matextrem
[Explorer] Remove 'This project is in beta. Use at your own risk' mes…
d9831ca 
…sage in footer (#1036)

# Summary

Closes #1035 

Removed 'This project is in beta. Use at your own risk' message in footer

![image](https://user-images.githubusercontent.com/11525018/152871045-a022cffe-71c8-48e9-a180-7dc0e0a14222.png)


# To Test

1. Open any page. i.e: `Home`
    * You'll see the message 'This project is in beta. Use at your own risk' in footer has been removed.
@matextrem
Add LogoWrapper component + logos assets
f9001e8
@matextrem
Merge branch 'develop' of github.com:gnosis/gp-ui into develop
6c26243
@matextrem
Add github + etherscan icons
097fed7
@github-actions
Copy link

github-actions bot commented Feb 9, 2022

@elena-zh
Copy link

Hey @matextrem , great changes!
However, I have some doubts about the UI itself.
So it would be nice to update the brunch and include the fix where 'This project is in beta. Use at your own risk.' message is removed, so I will be able to see how the elements are placed without this message.
Currently, I'd increase margin between contracts, and make elements look nicer in the mobile view:
mobile
Screenshot_2

Another case I'd like to discuss is icons placing at the top of User/transaction details pages: I personally think that copy icon should be placed near the address, so maybe we could replace icons, or move Etherscan icon before an addres/TxHash?
another replace
replace

@alfetopito , @alongoni , WDYT?

@alongoni
Copy link
Contributor

alongoni commented Feb 10, 2022 •
edited
Loading

Hey @matextrem It's looking great. I'm agree with @elena-zh
Also I'll add some notes:

  • Add a title to the image tag (<img src="logo.svg" title="Open it on Etherscan"> or something like that. Same case for Github logo).
  • As Elena mention, we can move the Etherscan logo at the begining of the address link.
  • I was thinking what about using a different version of the Etherscan logo in white. Like the last of the list: https://etherscan.io/brandassets

@alfetopito
Copy link
Contributor

Now that we are talking about it, I think it makes more sense to do like CowSwap footer and have both links (blockexplorer + github) for each contract

Screen Shot 2022-02-10 at 07 30 22

Right now we have the block explorer for both, but the github link points only to the repo, not to the respective files.

Not saying it should be identical (same orientation and positioning), but maybe we should be in sync with what's displayed:

  • 2 entries per contract, like now, but instead each have 1 link to blockexplorer and 1 to github
  • 1 entry for the UI github

Now, regarding the icon on top. I'm not convinced we should move the icon to the left.
I find it ok on the right side before the copy button.

@biocom would you please share your wisdom with on the matter?

@matextrem
Copy link
Contributor Author

@alfetopito where can I get the GitHub links for the contracts?

@alfetopito
Copy link
Contributor

@alfetopito where can I get the GitHub links for the contracts?

CowSwap :)

https://github.com/gnosis/cowswap/blob/4fb7795a5c9d71c348433044f620cf34dd7a9b9e/src/custom/components/Version/index.tsx#L41-L50

@elena-zh
Copy link

Changes LGTM.
But still I'm a bit concerned with a mobile footer view: would it be better to place all footer elements one under another in order to avoid overlappings/paddings issues?
overlap
image
image

@alongoni , WDYT?

@alongoni
Copy link
Contributor

Changes LGTM. But still I'm a bit concerned with a mobile footer view: would it be better to place all footer elements one under another in order to avoid overlappings/paddings issues? overlap image image

@alongoni , WDYT?

Yes! Perhaps if we have mobile resolutions under ~360px, we can set all footer links in one column.

@alfetopito
Copy link
Contributor

One last thing. Since now there are two entries for the contract, I think we no longer need the standalone link Contracts: v1.1.2. It can be removed.

@elena-zh
Copy link

Hey @matextrem , still I can find some nitpicks in the PR:

  1. Margin between footer elements is different:
  • [430-633] px:
    image
  • 429 px and less:
    image

  1. Still I can face overlapping in a mobile resolution:
    image

  2. I would align elements to the left when they are displayed one under another. Besides, it would be nice to lake margins between elements the same.
    image

Could you please take a look at these issues?

elena-zh
elena-zh approved these changes Feb 15, 2022
View reviewed changes
Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

LGTM now!

alongoni
alongoni approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Looks great!

@matextrem matextrem self-assigned this Feb 16, 2022
@matextrem matextrem added Protofire task to the protofire team Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds labels Feb 16, 2022
@mergify mergify bot merged commit f5c323a into develop Feb 16, 2022
@alfetopito alfetopito deleted the 1021/Add-text-externalLinks branch February 17, 2022 17:29
This was referenced Apr 1, 2022
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alfetopito alfetopito alfetopito approved these changes

@alongoni alongoni alongoni approved these changes

@elena-zh elena-zh elena-zh approved these changes

Assignees

@matextrem matextrem

Labels
Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds Protofire task to the protofire team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add 'View on Etherscan/Blockscount' to external links
4 participants
@matextrem @elena-zh @alongoni @alfetopito