-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
docs(v2): Nav links activeBasePath #2303
Conversation
Deploy preview for docusaurus-2 ready! Built with commit 16b5f10 |
const activeBaseProps = | ||
activeBaseRoute != null | ||
activeBasePath != null |
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 wonder why not use strict comparison here?
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.
Because I want to check both null
and undefined
. != null
checks for both.
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 would then remove this check altogether, but ok.
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 guess I can do that. The other falsey values aren't valid path values anyway.
website/docusaurus.config.js
Outdated
label: 'Docs', | ||
position: 'left', | ||
}, | ||
{to: 'blog', label: 'Blog', position: 'left'}, | ||
{to: 'blog', activeBasePath: 'blog', label: 'Blog', position: 'left'}, |
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.
activeBasePath: 'blog'
It’s not necessary here, the Blog link is highlighted perfectly without it, isn’t it?
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.
For now, nope. But we never know if that might change in future. Anyway no harm adding IMO. I'm also ok with removing but I think it's good for education.
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 seems to me a "bad example" in this case, we should show that the activeBasePath
option is more of a workaround that should be used wisely.
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.
Sure, will remove.
Motivation
Add docs for #2299. I decided to rename
activeBaseRoute
toactiveBasePath
.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Just docs...
Related PRs
#2299