-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Remove external link icon and update docs #6518
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
Conversation
size-limit report 📦
|
a3a40bb to
0281d22
Compare
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
chloerice
left a comment
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.
Thanks for tackling this @sarahill!! The guidance you added for this is excellent 🙌🏽 🚀 Just made a few non-blocking copy suggestions.
Co-authored-by: Chloe Rice <[email protected]>
Co-authored-by: Chloe Rice <[email protected]>
Co-authored-by: Chloe Rice <[email protected]>
Co-authored-by: Chloe Rice <[email protected]>
…laris into external-link-updates
jjgali
left a comment
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.
Left one tiny comment—looks fantastic!
|
|
||
| Avoid using the [external link icon](/icons?icon=ExternalMinor&q=exter), as it can add unnecessary visual load inside a sentence or when accompanied by other content. Instead, add clarity to external links through clear link text and predictable placement of the link in a merchant’s workflow. | ||
|
|
||
| Edge case: External icons should not be used as an indicator that a new tab or window will be opened. However, they may be used sparingly in features where symbols help merchants scan and pick from a list of several kinds of navigation options, like the admin's global search results. |
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 this be “External link icons”?
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.
Good question. The icon name is "external" so that's why we'd went with that but they are just used for external links right now. The thing that is a little confusing is that the "Promote" icon looks an awful like the external icon 🤔
http://localhost:3000/icons?icon=ExternalSmallMinor&q=external
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.
A gotcha—if that's the icon name, then that makes sense! I thought it was “external link icon” based on line 63. Also, I think the link may be broken on that same line. Looks like the end of the URL got cut off.
|
A few small comments:
|
|
@emily-broughton I made the changes you suggested! Thank you 👏 |
emily-broughton
left a comment
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.
Looks good to me!
WHY are these changes introduced?
Fixes #6413
External link example

New guidance in best practices

WHAT is this pull request doing?
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes