fix(v2): improve dx sidebar config, ability to have no sidebars file #4775
Conversation
|
[V2] Built with commit 0b82fd0 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4775--docusaurus-2.netlify.app/ |
nam-hle
force-pushed
the
improve-sidebar-config-dx
branch
from
faff8c9 to
6e28fdf
Compare
| @@ -93,7 +93,10 @@ export function cliDocsVersionCommand( | |||
| } | |||
|
|
|||
| // Load current sidebar and create a new versioned sidebars file. | |||
| if (fs.existsSync(sidebarPath)) { | |||
| if ( | |||
There was a problem hiding this comment.
would be simpler to handle the false value in loadSidebars
There was a problem hiding this comment.
I don't feel the existsSync call is really useful btw, if the user provided a bad path an error is already displayed when he starts his site
| @@ -249,7 +252,8 @@ export default function pluginContentDocs( | |||
| return { | |||
| ...versionMetadata, | |||
| mainDocId: getMainDoc().unversionedId, | |||
| sidebars, | |||
| sidebars: | |||
| versionMetadata.sidebarFilePath === false ? undefined : sidebars, | |||
There was a problem hiding this comment.
not needed if loadSidebars handles false and return an empty object
| sidebarFilePath: string | false | undefined, | ||
| ): UnprocessedSidebars { | ||
| // In case false value, we have to set to DefaultSidebars first to generate navigation link | ||
| // See https://github.com/facebook/docusaurus/pull/4775 |
There was a problem hiding this comment.
not sure what you mean with that comment. Imho we just need to return an empty object if the value is false
There was a problem hiding this comment.
If we return empty object, so there is no previous/next button in the end of the doc. That's why I need to do so.
There was a problem hiding this comment.
but if the user uses false, there should be no navigation, I believe this is the expected behavior. There is only a navigation if there is a sidebar, and we should make sure that when false is used, there is no navigation
| @@ -183,7 +182,9 @@ function getVersionMetadataPaths({ | |||
| versionName, | |||
| }); | |||
|
|
|||
| const sidebarFilePath = isCurrentVersion | |||
| const sidebarFilePath = !options.sidebarPath | |||
There was a problem hiding this comment.
I don't think this condition is correct, the "main test" should remain based on the isCurrentVersion. Versioned docs should always have a sidebar file available
There was a problem hiding this comment.
Hmm actually not: versioned files can have no sidebars file, so we may compute a path for them but in the end the file may not exist and we shouldn't warn.
This will be better:
function getSidebarFilePath() {
if (isCurrentVersion) {
return options.sidebarPath ? path.resolve(context.siteDir, options.sidebarPath) : options.sidebarPath
} else {
return path.join(
getVersionedSidebarsDirPath(context.siteDir, options.id),
`version-${versionName}-sidebars.json`,
);
}
}
| if (!fs.existsSync(sidebarFilePath)) { | ||
| console.log( |
There was a problem hiding this comment.
going to move this warning (and make it an error), to its original place, as we should throw in case of file not found, but not for the versioned docs (as a missing file is a possibility). Unfortunately in this function we don't have the current version name so it's not the best place to handle the error.
|
[V1] Built with commit 0b82fd0 |
| @@ -20,6 +20,67 @@ import { | |||
| import {loadSidebars} from './sidebars'; | |||
| import {DEFAULT_PLUGIN_ID} from '@docusaurus/core/lib/constants'; | |||
|
|
|||
| function createVersionedSidebarFile({ | |||
There was a problem hiding this comment.
FYI refactored and extracted a method here
|
|
||
| // Do not create a useless versioned sidebars file if sidebars file is empty or sidebars are disabled/false) | ||
| const shouldCreateVersionedSidebarFile = | ||
| Object.keys(loadedSidebars).length > 0; |
There was a problem hiding this comment.
this is the important change: should we create a file in versioned-sidebars folder if the user has disabled the sidebars or is using an empty sidebars.json?
It used to not be created in some cases so for retrocompatibility I think it make sense to keep not creating it if it would lead to an empty file
|
Thanks for your help on this @nam-hle ! Changed a few things and now it looks good to merge |
slorber
changed the title
Motivation
From #4752.
What the PR does
sidebarsPathfalse(to disable the sidebars),undefinedto generate automatically, the wrong path will report a warning message for example:With
docusaurus docs:versioncommand:false:copy docs files toversioned-docsonlyundefinedor wrong path: copy docs files toversioned-docs, create auto-generated version sidebars file to versioned-sidebars.Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan