-
-
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
Finish go to selected paged from category sidebar #4323
Conversation
[V1] Deploy preview failure Built without sensitive environment variables with commit e2998f7 https://app.netlify.com/sites/docusaurus-1/deploys/60411f70eaefbc00073fd738 |
Deploy preview for docusaurus-2 failed. Built without sensitive environment variables with commit e2998f7 https://app.netlify.com/sites/docusaurus-2/deploys/60411f70737bf200070b220e |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4323--docusaurus-2.netlify.app/classic/ |
Size Change: 0 B Total Size: 532 kB ℹ️ View Unchanged
|
packages/docusaurus-theme-classic/src/theme/DocSidebar/index.tsx
Outdated
Show resolved
Hide resolved
packages/docusaurus-theme-classic/src/theme/DocSidebar/index.tsx
Outdated
Show resolved
Hide resolved
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 working on this
Left some inline review comments
let me know if you need any help to handle those
packages/docusaurus-theme-classic/src/theme/DocSidebar/index.tsx
Outdated
Show resolved
Hide resolved
if (Object.hasOwnProperty.call(item.link, 'type')) { | ||
switch (item.link.type) { | ||
case 'doc': | ||
setInitialLink(item.link.id); |
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 is not correct, the id is not the URL path, as docs can use slugs they have ids that can be totally different from their final URL.
This requires resolving the id to the doc URL path
(+ shouldn't need setState)
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.
Is there a function for getting the URL path from a document id?
packages/docusaurus-plugin-content-docs/src/plugin-content-docs.d.ts
Outdated
Show resolved
Hide resolved
@@ -97,7 +97,7 @@ function assertItem<K extends string>( | |||
function assertIsCategory( | |||
item: unknown, | |||
): asserts item is SidebarItemCategoryJSON { | |||
assertItem(item, ['items', 'label', 'collapsed', 'customProps']); | |||
assertItem(item, ['items', 'label', 'collapsed', 'link', 'customProps']); |
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.
We need to validate that the link attribute is valid in this function + add unit tests
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 might need some help on this one
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 working on this
Left some inline review comments
let me know if you need any help to handle those
Yeah, I think I'll need help on what's left. Mostly decoding the url from the doc id and validation on the link. Thanks
packages/docusaurus-theme-classic/src/theme/DocSidebar/index.tsx
Outdated
Show resolved
Hide resolved
if (Object.hasOwnProperty.call(item.link, 'type')) { | ||
switch (item.link.type) { | ||
case 'doc': | ||
setInitialLink(item.link.id); |
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.
Is there a function for getting the URL path from a document id?
Hi, what's the status of this PR? I'd be really looking forward to this feature. @ben-qnimble In case you are still working on it, here are a few hints:
You need several hooks to extract this information, but here's a reference: docusaurus/packages/docusaurus-theme-classic/src/theme/NavbarItem/DocNavbarItem.tsx Lines 49 to 62 in 295e77c
Just follow the pattern below to verify that I'm in need of this feature and it's a shame seeing it getting stalled multiple times now 🤦♂️ |
@@ -132,6 +153,7 @@ function DocSidebarItemCategory({ | |||
<DocSidebarItem | |||
tabIndex={collapsed ? '-1' : '0'} | |||
key={childItem.label} | |||
link={link} |
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.
Why bind this to the children elements? Only the category with link
attribute should have a link associated right?
This is a complex feature and this PR is not really in the direction I'd like to take to provide a set of useful features to Docusaurus (in particular #2643) I'll try to see if there's anything to take here, but ultimately it's likely that I close it. I'll start to work on these features soon, this is the next big task on my todolist. |
Closing in favor of #5830 |
Motivation
Pull #3898 looks very helpful (sidebar category linking to doc) but it seems like it got stalled, so I'm trying to get it over the finish line by addressing issues mentioned in the pull request code review.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Please see #3898. I set a few categories to have links and verified that they work as expected.
Related PRs
Adding to the commits in #3898 to address code review comments.