-
Notifications
You must be signed in to change notification settings - Fork 464
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
feat: redesign (external) links #1241
Conversation
ESLint Summary View Full Report
Report generated by eslint-plus-action |
@@ -488,7 +488,7 @@ const initTheme = (darkMode: boolean) => { | |||
MuiLink: { | |||
styleOverrides: { | |||
root: ({ theme }) => ({ | |||
textDecoration: 'none', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked about this change with lila
Branch preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good but why does the component need to be "external" specific. Surely internal links need to look the same
target="_blank" | ||
sx={{ textDecoration: 'none' }} | ||
> | ||
<ExternalLink href="https://safe.global/terms" sx={{ textDecoration: 'none' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the textDecoration
now?
Does this also resolve #1231 or are there gnosis-safe.io links left? |
It should at least I searched for gnosis-safe.io and tried to update all links which should point to safe.global. I left some urls which are pointing to the old app on purpose. I'll edit the PR description. Didnt see that issue :) |
* It also always adds the external link icon as end adornment. | ||
*/ | ||
const ExternalLink = ({ | ||
suppressIcon = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would name this hideIcon
for brevity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Branch preview✅ Deploy successful! https://redesign-external-links--webcore.review-web-core.5afe.dev |
CLA Assistant Lite All Contributors have signed the CLA. |
Hey @francovenica, I merged dev into this branch and fixed multiple links in the old and new safe creation to use the new ExternalLink component. Good catch. Btw in the old safe creation those two links you found did not even link to any pages. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Looks good. |
What it solves
Links look the same as normal text making it hard to see what is a clickable link and what is just text.
Resolves #1174 , #1231
How this PR fixes it
ExternalLink
component which is similar to Link but always sets a securerel
attribute, the target to_blank
and by default appends an external-link icon.gnosis-safe.io
links bysafe.global
. For instance links to help articles, policies, etc.How to test it
Check the various external links in our interface:
Analytics changes
None
Screenshots