-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: open docs links in the same tab closes #6927 #6936
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I'm getting a 404 on this current approach. https://nodejs-org-git-fork-wesleytodd-docs-in-tab-openjs.vercel.app/en/docs/latest/api/ It may need to be the absolute url to account for the way the site is built and/or requests are routed by CF |
@wesleytodd Does it work locally ? |
This works, "docs": {
"link": "https://nodejs.org/docs/latest/api/",
"label": "components.containers.navBar.links.docs"
}, But, the icon in navbar renders. |
With absolute paths, it will always use i18n routing. That's why we use the full path. We can use the full DN (https://nodejs.org/api...), but the external icon will render it, although it will not open on a blank tab, which is what this PR intends to fix. |
Well IMO the icon should also not render if it is not opening in a new tab. @ovflowd, am I understanding this last comment that the current setup would need a change to exclude this route from the i18n treatment which adds in the |
Figured it was just easier to push a change to this and see what y'all wanted to do. |
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'm fine with this. From a user perspective I wouldn't imagine anyone sees these as two different sites, and someone can always middle click or "open in new tab/window" contextually.
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.
LGMT !
I prefer this behavior
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!
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, let's land this
Description
Open Docs link in the same tab
Validation
Open the docs link in the header, should not have the icon and should open in the same tab.
Fixes #6927
Related Issues
Check List
npm run format
to ensure the code follows the style guide.npm run test
to check if all tests are passing.npx turbo build
to check if the website builds without errors.