-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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(v2): add icon to external navbar links #4949
Conversation
@@ -32,6 +35,8 @@ function NavLink({ | |||
const toUrl = useBaseUrl(to); | |||
const activeBaseUrl = useBaseUrl(activeBasePath); | |||
const normalizedHref = useBaseUrl(href, {forcePrependBaseUrl: true}); | |||
const isExternalLink = label && href && href !== '#'; |
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.
Curious, if we should consider #
as internal link? If so, we can use the built-in isInternalUrl
function instead this condition.
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.
seems reasonable yes, just made the change
However I find this code a bit messy, don't really like that to/href are handled differently, we should try to make all this more uniform in the future (this is needed anyway for markdown as you can't precise if links are to or href)
✔️ [V2] 🔨 Explore the source changes: 93dbe61 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60c8950870a0230008d1563a 😎 Browse the preview: https://deploy-preview-4949--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4949--docusaurus-2.netlify.app/ |
Size Change: +2.39 kB (0%) Total Size: 623 kB
ℹ️ View Unchanged
|
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.
Not sure all users will appreciate having an external link icon in their navbar, some sites have a lot of external links in navbar and having this icon multiple times in navbar does not look too good imho. + I don't think there's a clear API to disable this apart swizzling.
What about limiting this for dropdown items only and not top-level navbar items? (at least for desktop?)
@@ -32,6 +35,8 @@ function NavLink({ | |||
const toUrl = useBaseUrl(to); | |||
const activeBaseUrl = useBaseUrl(activeBasePath); | |||
const normalizedHref = useBaseUrl(href, {forcePrependBaseUrl: true}); | |||
const isExternalLink = label && href && href !== '#'; |
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.
seems reasonable yes, just made the change
However I find this code a bit messy, don't really like that to/href are handled differently, we should try to make all this more uniform in the future (this is needed anyway for markdown as you can't precise if links are to or href)
I was inspired by VuePress as example that adds icon for external links to the navbar. Actually, end user can remove this icon via CSS attribute selector. I suggest adding this icon without explicit customization for now, and just wait for feedback from users. As for me, it's not a big change, so I do not think that there will be a lot of dissatisfied with it. |
Yes, will remove this link, it was added just for demo purposes. However, in Infima I previously opened PR that reduces current padding between navbar items. This seems to make sense to me, can you check this out? |
Motivation
For more consistency, we should probably add icon for external links in the navbar too. This was previously done for sidebar links (see #4261).
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Preview
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)