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

Fix block explorer links #411

Merged
merged 10 commits into from Nov 21, 2022

Conversation

rulfo71
Copy link
Contributor

@rulfo71 rulfo71 commented Nov 7, 2022

Description

Fixing block explorer links through the app

Closes #338

image
image

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Manually for now

Checklist:

  • My code follows the existing style of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any UI changes have been tested and made responsive for mobile views

@github-actions
Copy link

github-actions bot commented Nov 7, 2022

@github-actions
Copy link

github-actions bot commented Nov 7, 2022

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeibawkzldkgilc7lnb5ryxxx5lpfk5pxotpqato44onzbmiw6zo6ve.ipfs.cf-ipfs.com

@Filipv95
Copy link
Contributor

Filipv95 commented Nov 7, 2022

Hey @rulfo71, is this change applying only on addresses withing expanded Proposal Action on Proposal page?

@rulfo71
Copy link
Contributor Author

rulfo71 commented Nov 8, 2022

hey @Filipv95 !
for now it's done only for the one you mentioned, on create proposal page but when i finish it it will be for all addresses

@rulfo71
Copy link
Contributor Author

rulfo71 commented Nov 8, 2022

at the moment is finished but missing tests on component BlockExplorerLink

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeiho3qx2swidyh6jfnmw3kfm3g7fyaxebg4bn5vhrkh5htaaofxxy4.ipfs.cf-ipfs.com

Comment on lines 3 to 30
export const StyledSegmentLink = styled.a`
color: ${({ theme }) => theme.colors.grey};
margin-right: 0.5rem;
display: flex;
justify-content: space-between;
align-items: center;
text-decoration: none;
overflow-wrap: break-word;
&:hover {
cursor: pointer;
color: ${({ theme }) => theme.colors.text};
}
`;

export const UnstyledExternalLink = styled.a`
text-decoration: none;
display: flex;
justify-content: space-between;
align-items: center;
color: ${({ theme }) => theme.colors.text};
&:focus,
&:hover,
&:visited,
&:link,
&:active {
text-decoration: none;
}
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we unify this in one component, passing the unstyled prop, and handle the styling logic here?

src/components/primitives/Links/ExternalLink.tsx Outdated Show resolved Hide resolved
return (
<StyledSegmentLink href={href} target="_blank" rel="noopener">
{children}
<BiLinkExternal />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<BiLinkExternal />
<FiLinkExternal />

src/components/primitives/Links/ExternalLink.tsx Outdated Show resolved Hide resolved
@github-actions
Copy link

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeifxgpbxf7ldl2aa5g7dnjghklqbuczjfjndn76umfgvmiyr4vy6fi.ipfs.cf-ipfs.com

const blockExplorerUrl = getBlockExplorerUrl(chain, address, 'address');

return (
<FlexContainer>
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a <Flex /> component in components/primitives/Layout/Flex.

We should use that instead, and also remove the component from the ./style file

Copy link
Contributor Author

@rulfo71 rulfo71 Nov 11, 2022

Choose a reason for hiding this comment

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

great to know

Comment on lines +18 to +20
export const FlexContainer = styled.div`
display: flex;
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const FlexContainer = styled.div`
display: flex;
`;
export const FlexContainer = styled.div`
display: flex;
`;

Removing this (see comment above)

Comment on lines 11 to 25
if (unstyled) {
return (
<StyledSegmentLink href={href} target="_blank" rel="noopener">
<LinkDetail>{children}</LinkDetail>
<FiExternalLink />
</StyledSegmentLink>
);
}

return (
<StyledSegmentLink href={href} target="_blank" rel="noopener">
<LinkDetail>{children}</LinkDetail>
<FiExternalLink />
</StyledSegmentLink>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be only one component and handle the styling logic in the styled component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! yes i didn't realize this is not needed anymore.

@github-actions
Copy link

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeifdg6bpwfkkl35g5jbbji5txi476vjx7dzrz5whygaixcwe3jl744.ipfs.cf-ipfs.com

dcrescimbeni
dcrescimbeni previously approved these changes Nov 11, 2022
@github-actions
Copy link

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeibpay3perbqa4ec56fnlg3f64b5y3f2343ral7wcm2mnas25b6rgq.ipfs.cf-ipfs.com

dcrescimbeni
dcrescimbeni previously approved these changes Nov 11, 2022
@rulfo71 rulfo71 changed the title [WIP] Fix block explorer links Fix block explorer links Nov 14, 2022
@github-actions
Copy link

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeidc2ydhxkszblxyy7eokvvw76q726jo6oa7ihrdanni3ifpha5jyi.ipfs.cf-ipfs.com

0xvangrim
0xvangrim previously approved these changes Nov 16, 2022
Copy link
Contributor

@0xvangrim 0xvangrim left a comment

Choose a reason for hiding this comment

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

Clean! 👍

src/components/primitives/Links/BlockExplorerLink.tsx Outdated Show resolved Hide resolved
src/components/primitives/Links/BlockExplorerLink.tsx Outdated Show resolved Hide resolved
@github-actions
Copy link

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeid4hgk2dwfjnpdmhin5ztodhskcrrtkjchztycmwu5wkdf3vnw424.ipfs.cf-ipfs.com

@github-actions
Copy link

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeiep4cjy4icessbo4mdqv62tikmeb4vxkxhg7ymkbmwk6bsqo7h4ti.ipfs.cf-ipfs.com

@rossneilson
Copy link
Contributor

Could we also change blockscout to link to gnosis scan https://gnosisscan.io/

@github-actions
Copy link

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeiczcotadzs4rmymjssx3rupa45le32x25vz2bnlcdjd5mcwwjwq6y.ipfs.cf-ipfs.com

@rulfo71 rulfo71 merged commit ca435af into DXgovernance:develop Nov 21, 2022
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 this pull request may close these issues.

Fix block explorer links
6 participants