-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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(docs,theme-classic): docs breadcrumbs #6517
Conversation
✔️ [V2] 🔨 Explore the source changes: df6ef4c 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/620d38a444330700077471cd 😎 Browse the preview: https://deploy-preview-6517--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6517--docusaurus-2.netlify.app/ |
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, that looks like a useful feature to add and the implementation looks good overall
Added some comments.
I'm not sold on the nested
config, maybe we could figure out a better name? 🤷♂️
Do we want this config to be themeConfig or plugin option? (ie 2 plugins can have different options? 🤪 )
What if later we add breadcrumbs to blog posts? (like 2021 > Docusaurus Recap
or something), will the themeConfig still make sense in such case? Shouldn't we make it clear that it only impacts the docs?
const {pathname} = useLocation(); | ||
const breadcrumbs: BreadcrumbItem[] = []; | ||
|
||
function extract(items: PropSidebar) { |
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 have a hook useCurrentSidebarCategory
already
I'd rather create another generic one called useCurrentSidebarItem
+ findSidebarItem
(pure fn that can be easily tested independently)
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.
Might need some more info here - this extract
fn is called recursively to pull out the sidebar item for each level, but it sounds like the purpose of useCurrentSidebarCategory
(and what you're suggesting with useCurrentSidebarItem
) is to extract just the category/item for the current page. Is that correct?
packages/docusaurus-theme-classic/src/theme/DocBreadcrumbs/index.tsx
Outdated
Show resolved
Hide resolved
14bd0e8
to
0ce2205
Compare
@slorber I like this approach. I am having a little trouble understanding how to feed this plugin option down to theme-classic, though. As a theme option I just used the I will take another look shortly, but if it's simple would you be able to point me in the right direction? |
@jodyheavener the plugin options are not all serializable and not all forwarded to the client (unlike the theme config) To provide this option to the frontend, you should use the global data API so that you receive this data with setGlobalData<GlobalPluginData>({
path: normalizeUrl([baseUrl, options.routeBasePath]),
versions: loadedVersions.map(toGlobalDataVersion),
+ breadcrumbs: options.breadcrumbs,
}); Then you can create the hook to read this API: packages/docusaurus-plugin-content-docs/src/client/index.ts export const useDocsBreadcrumb = (pluginId: string | undefined): DocsBreadcrumb =>
useDocsData(pluginId).breadcrumbs; There's one issue though: how do you pass the pluginId so that you get the correct value? 🤷♂️ For now you can keep it simple and pass undefined, I'll figure out a way to provide the id, it requires a core API that I want to add anyway, something like |
Oh actually the docs plugin already permits to retrieve the current active plugin! forgot about this API. Once you setup global data like above, this should work: useActivePlugin().pluginData.breadcrumbs |
321dcfe
to
36ff594
Compare
That's generally true, but not in this case. This option is serializable and actually available with |
36ff594
to
bc5287d
Compare
@slorber I went ahead and made the majority of your changes, left you one more question. While I was playing around I experimented with other placements. Should there be an option to choose if the breadcrumbs appear above or below the document title? |
Thanks @jodyheavener that looks good to me 👍 I'm going to run this locally and complete some minor details
I think we should not provide such an option. It's complicated to provide good options for everything possible layout combinations. At the end of the day, users that have different opinions should rather use swizzle and have full flexibility. If we add another option, it would only be a "shortcut" so we must rather be sure that it's a common pattern worth adding an option for in the first place |
@jodyheavener what do you think about removing "nested" and instead having this? I find this particularly weird that some pages have a breadcrumb and some others don't 🤷♂️ , it produces an inconsistent layout experience across pages that gives a weird feeling, as headings shift when you navigate |
@slorber Oh I looove that, so much better than inconsistent presence of breadcrumbs, and solves the problem of only having a single breadcrumb on top-level pages. 👏 |
great, so let's do this and we'll eventually see if anyone find a good use-case for an extra option ;) |
@slorber awesome, thanks so much for your input and changes with this :) |
np, LGTM 👍 |
Apparently the home breadcrumb item is a problem for some sites because they don't have a homepage 😅 Maybe we could hide the home breadcrumb item if there is no homepage (page or docs or blog)? This would lead to only 1 item UX that we wanted to avoid but only for edge cases so that seems acceptable I also like this behavior of the React beta site: https://beta.reactjs.org/learn/render-and-commit Somehow it renders the sidebar as the breadcrumb parent element, that's another interesting thing we could explore Problem: we don't have a way to assign a label to the sidebar, that would require another change to the sidebar format 😅 but that seems like a useful change to do in the long term so that sidebars can have more global per-sidebar configs Any opinion? |
We need labels for navbar sidebar items as well. Indeed, this is hard to design, and we don't want to further complicate the sidebar structure. I find the shorthand notation very useful and I personally use it a lot in my websites, but it's almost impossible to keep if we want to mix metadata with sidebar items. What about exporting a separate sidebar metadata object in |
That seems like a bad but retro compatible workaround, we would probably never design this API this way if implemented from scratch 😅 |
True... What about this? I think this is what you are thinking of? Before module.exports = {
someSidebar: {
our: ['cool', 'shorthand'],
},
}; After module.exports = {
someSidebar: {
label: 'Some sidebar',
items: {
our: ['cool', 'shorthand'],
},
},
}; Going to be another huge breaking change :D |
Yes that's the idea We can probably support both at the same time, considering it's likely the user uses a top-level category Anyway, all this doesn't need to be implemented immediately, we should just fix the reported issue before next release for now |
Breadcrumbs were added in beta 16 and they are enabled by default https://twitter.com/docusaurus/status/1497229239921356801 facebook/docusaurus#6517
Motivation
There is a desire to have breadcrumbs visible in docs.
This PR introduces breadcrumbs in a few ways:
useDocsBreadcrumbs
hook - parses the current sidebar items for the active "path" of items, returning them in an array of objects containinglabel
and optionalhref
.breadcrumbs
theme configuration property - accepts a boolean value, or"nested"
to indicate that breadcrumbs should only be visible when not in a top-level page."nested"
(open to pushback on this, I just found it to be redundant to see a single breadcrumb item)DocBreadcrumbs
component - utilizes the new hook to retrieve breadcrumb items, rendering them in a navigation element.DocItem
andDocCategoryGeneratedIndexPage
.false
never renders, if"nested"
only renders if you are not on a top level page.href
s as<span>
s.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Here's how you can test this new feature.
"nested"
, so you should only see breadcrumbs on non top-level doc pages.true
should display breadcrumbs on all doc pages.false
should disable breadcrumbs altogether.<span>
element, not clickable.Here is a video demonstrating breadcrumbs in action. It includes the
nested
config, and goes over varying levels of nesting, across different versions, in mobile and desktop, and in light/dark mode.Screen.Recording.2022-01-30.at.15.33.05.mov
Open to feedback on how I could write better tests for this.
Related PRs
Closes #6096
😄 I took some liberties with this PR, and while I hope you like it I fully anticipate changes to be requested. Excited for your feedback!