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

chore: refactor explorer links and menu, add explorer in menu #2836

Merged
merged 3 commits into from
Jul 10, 2023

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Jul 10, 2023

Summary

Address the issue we didn't have a explorer link in the header

image

Implementation notes for reviewers

TBH i thought this would be a 5min thing, and it turned out I had to implement a few features in order to make it work.

Let me drive you through the thinking process.

  1. Our links are created by configuration of some const here

    { id: MainMenuItemId.MORE_ABOUT, title: 'About', url: Routes.ABOUT, iconSVG: IMAGE_INFO },

  2. The link to the explorer can't be added as a static type, because the URL changes for example between different networks

  3. I decided to make a new type of Menu Item with an arbitrary component, this way we can make any dynamic logic. This new type is:

  1. Then I have to adapt the <ExplorerLink /> component, because it was assumed we always use it for links with an id (a transaction, or address). In this case, i just want to send it to the home page of the explorer
    return getExplorerBaseUrl(chainId)
  1. Additionally, the link HTML was not good in this case, because i needed to have some image and custom markup for the link, so i gave children support

  2. with all this, implementing the link is just to:

Most of the changes will be to adapt the link parameters, because the id is now optional, it should be last

EDIT: adding default network

Before @elena-zh opens an issue, i noticed the link was not displayed if the network is unsupported

Now i created a parameter to the ExplorerLink component so it can have a default network

image

To Test

1 .Check the link in the different networks

@vercel
Copy link

vercel bot commented Jul 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview Jul 10, 2023 4:38pm

@anxolin anxolin requested review from a team July 10, 2023 16:39
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

@anxolin anxolin merged commit 883919c into hotfix/1.40.6 Jul 10, 2023
4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 2023
@alfetopito alfetopito deleted the refactor-links-explorer branch July 11, 2023 08:56
Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Approve, just one tiny comment

id: string
type?: BlockExplorerLinkType
interface PropsBase extends PropsWithChildren {
// type?: BlockExplorerLinkType
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead code?

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.

3 participants