sidebar: "hidden" #701
sidebar: "hidden" #701
Conversation
src/config.ts
Outdated
| @@ -107,7 +106,7 @@ export async function normalizeConfig(spec: any = {}, defaultRoot = "docs"): Pro | |||
| let {title, pages = await readPages(root), pager = true, toc = true} = spec; | |||
| if (title !== undefined) title = String(title); | |||
| pages = Array.from(pages, normalizePageOrSection); | |||
| sidebar = sidebar === undefined ? pages.length > 0 : Boolean(sidebar); | |||
| const {sidebar = "auto"} = spec; | |||
There was a problem hiding this comment.
This needs to validate the allowed values of sidebar (either "auto" or "hidden" or boolean). Meaning you should call maybeSidebar here, earlier.
Also, I think we should still default to false if pages.length === 0.
There was a problem hiding this comment.
oops yes, that's an oversight!
src/render.ts
Outdated
| @@ -59,7 +59,7 @@ type RenderInternalOptions = | |||
|
|
|||
| async function render(parseResult: ParseResult, options: RenderOptions & RenderInternalOptions): Promise<string> { | |||
| const {root, path, pages, title, preview} = options; | |||
| const sidebar = parseResult.data?.sidebar !== undefined ? Boolean(parseResult.data.sidebar) : options.sidebar; | |||
| const sidebar = maybeSidebar(parseResult.data?.sidebar ?? options.sidebar, pages); | |||
There was a problem hiding this comment.
You shouldn’t use ?? for optional arguments because it would mean that sidebar: null is ignored. You have to check strictly for undefined.
src/render.ts
Outdated
| } | ||
|
|
||
| function maybeSidebar(sidebar: string | boolean, pages): "hidden" | boolean { | ||
| if (sidebar === "auto") sidebar = pages.length > 0; |
There was a problem hiding this comment.
At this point sidebar isn’t normalized, so the "auto" check will miss e.g. "AUTO".
src/render.ts
Outdated
|
|
||
| function maybeSidebar(sidebar: string | boolean, pages): "hidden" | boolean { | ||
| if (sidebar === "auto") sidebar = pages.length > 0; | ||
| return typeof sidebar === "boolean" ? sidebar : (keyword(sidebar, "sidebar", ["hidden"]) as "hidden"); |
There was a problem hiding this comment.
This will throw an error on sidebar: null, but I think we want to coerce any non-string value to a boolean. Something like:
| return typeof sidebar === "boolean" ? sidebar : (keyword(sidebar, "sidebar", ["hidden"]) as "hidden"); | |
| return typeof sidebar === "string" ? keyword(sidebar, "sidebar", ["auto", "hidden"]) : Boolean(sidebar); |
And I think we can have TypeScript assert that keyword returns one of the passed-in string literals using generics.
Co-authored-by: Mike Bostock <[email protected]>
Co-authored-by: Mike Bostock <[email protected]>
add tests
|
ptal |
There was a problem hiding this comment.
Hmm, in further testing, this doesn’t seem to persist the sidebar state in the session: if you visit a page and toggle the sidebar open, and then reload, the sidebar disappears; likewise if you navigate between pages, the sidebar keeps disappearing. I think that’s because we clear the session state if the sidebar would be initially visible (ignoring the hidden setting). In other words, the client needs to check the data-hidden attribute when deciding whether to persist the sidebar state in session storage.
|
Converting to draft given the needed changes. |
For #696. Note: as I was writing the documentation I thought that "closed" would be a better word. Rather than "hide" and "show" the sidebar, we'd open and close it. wdyt?