-
-
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): automatically add base url to logo link #2818
Conversation
Deploy preview for docusaurus-2 ready! Built with commit ca2f8e3 |
@@ -15,7 +15,7 @@ const useLogo = () => { | |||
siteConfig: {baseUrl, themeConfig: {navbar: {logo = {}} = {}}} = {}, | |||
} = useDocusaurusContext(); | |||
const {isDarkTheme} = useThemeContext(); | |||
const logoLink = logo.href || baseUrl; | |||
const logoLink = useBaseUrl(logo.href || baseUrl); |
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.
Wouldn't the useBaseUrl(baseUrl)
case result in duplication in the case of non-/
baseUrl
s?
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 just be wrapping around logo.href
?
const logoLink = logo.href ? useBaseUrl(logo.href) : baseUrl;
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.
Oops, fixed.
I set /
as the default value which will be processed by useBaseUrl
docusaurus/packages/docusaurus/src/client/exports/useBaseUrl.ts
Lines 30 to 32 in be7367b
if (url.startsWith('/')) { | |
return baseUrl + url.slice(1); | |
} |
Motivation
This is a request from Discord chat, I think we should take care to add the base url to the logo link automatically, this is the expected behavior I suppose.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Set document id to
navbar.logo.href
and make sure base url has been added to it.By default a slash (
/
) will be added (standard base url).Nevertheless, this is BC, because previously users had to manually specify the base url (in the case of using non-standard base url)
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.)