Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sidebar: "hidden" #701

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

sidebar: "hidden" #701

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
486ed02
sidebar option
mbostock Feb 7, 2024
81c5c60
sidebar docs
mbostock Feb 7, 2024
518333d
sidebar: "hidden"
Fil Feb 7, 2024
de8f96d
undo prettier
Fil Feb 7, 2024
25b5626
hasAttribute
Fil Feb 9, 2024
be782de
hasAttribute
Fil Feb 9, 2024
f29fc43
address comments from review
Fil Feb 9, 2024
5c32432
minimize diff
Fil Feb 9, 2024
b2e9053
Merge branch 'main' into fil/sidebar-hidden
Fil Feb 12, 2024
a3e259d
Merge branch 'main' into fil/sidebar-hidden
mbostock Mar 25, 2024
524e434
remove generic type
mbostock Mar 25, 2024
4ee364e
auto-hidden; mergeSidebar
mbostock Mar 25, 2024
95d7e8b
no “auto” sidebar
mbostock Mar 25, 2024
fb725d1
fixes
mbostock Mar 25, 2024
2dcca32
Merge branch 'main' into fil/sidebar-hidden
mbostock Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,19 @@ In this case, the path to the stylesheet is resolved relative to the page’s Ma

The project’s title. If specified, this text is used for the link to the home page in the sidebar, and to complement the titles of the webpages. For instance, a page titled “Sales” in a project titled “ACME, Inc.” will display “Sales | ACME, Inc.” in the browser’s title bar. If not specified, the home page link will appear as “Home” in the sidebar, and page titles will be shown as-is.

## sidebar

Whether to create and show the sidebar. One of:

- **true** — create a sidebar
- **false** — do not create a sidebar
- **auto** — default, create a sidebar if **pages** is not empty.
- **hidden** — create a hidden sidebar

By default, the sidebar is hidden on smaller screens and displayed on larger screens. However, the user can choose to display or hide it with a button and shortcut key (meta-B), and this choice is respected during the [page session](https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage).

If the option is set to _hidden,_ the sidebar always starts up hidden, requiring a user action to display it.

## pages

An array containing pages and/or sections. If not specified, it defaults to all Markdown files found in the source root in directory listing order.
Expand All @@ -118,6 +131,7 @@ export interface Page {
path: string;
}
```

Fil marked this conversation as resolved.
Show resolved Hide resolved
```ts run=false
export interface Section {
name: string;
Expand Down
1 change: 1 addition & 0 deletions src/client/sidebar-init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const toggle = document.querySelector<HTMLInputElement>("#observablehq-sidebar-t
// Restore the sidebar state from sessionStorage, or set it to indeterminate.
const initialState = sessionStorage.getItem("observablehq-sidebar");
if (initialState) toggle.checked = initialState === "true";
else if (toggle.getAttribute("data-hidden")) toggle.checked = false;
Fil marked this conversation as resolved.
Show resolved Hide resolved
else toggle.indeterminate = true;

// Restore the sidebar section state from sessionStorage, but don’t close any
Expand Down
6 changes: 4 additions & 2 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ export interface Config {
root: string; // defaults to docs
output: string; // defaults to dist
title?: string;
pages: (Page | Section)[]; // TODO rename to sidebar?
sidebar: "auto" | "hidden" | boolean; // auto defaults to true if pages isn’t empty
pages: (Page | Section)[];
pager: boolean; // defaults to true
scripts: Script[]; // defaults to empty array
head: string; // defaults to empty string
Expand Down Expand Up @@ -105,14 +106,15 @@ 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);
const {sidebar = "auto"} = spec;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops yes, that's an oversight!

pager = Boolean(pager);
scripts = Array.from(scripts, normalizeScript);
head = String(head);
header = String(header);
footer = String(footer);
toc = normalizeToc(toc);
deploy = deploy ? {workspace: String(deploy.workspace).replace(/^@+/, ""), project: String(deploy.project)} : null;
return {root, output, title, pages, pager, scripts, head, header, footer, toc, style, deploy};
return {root, output, title, sidebar, pages, pager, scripts, head, header, footer, toc, style, deploy};
}

function normalizeTheme(spec: any): string[] {
Expand Down
26 changes: 23 additions & 3 deletions src/render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type RenderInternalOptions =

async function render(parseResult: ParseResult, options: RenderOptions & RenderInternalOptions): Promise<string> {
const {root, path, pages, title, preview} = options;
const sidebar = maybeSidebar(parseResult.data?.sidebar ?? options.sidebar, pages);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn’t use ?? for optional arguments because it would mean that sidebar: null is ignored. You have to check strictly for undefined.

const toc = mergeToc(parseResult.data?.toc, options.toc);
return String(html`<!DOCTYPE html>
<meta charset="utf-8">${path === "/404" ? html`\n<base href="/">` : ""}
Expand Down Expand Up @@ -89,7 +90,7 @@ import ${preview || parseResult.cells.length > 0 ? `{${preview ? "open, " : ""}d
${
preview ? `\nopen({hash: ${JSON.stringify(parseResult.hash)}, eval: (body) => (0, eval)(body)});\n` : ""
}${parseResult.cells.map((cell) => `\n${renderDefineCell(cell)}`).join("")}`)}
</script>${pages.length > 0 ? html`\n${await renderSidebar(title, pages, path)}` : ""}${
</script>${sidebar ? html`\n${await renderSidebar(title, pages, path, sidebar)}` : ""}${
toc.show ? html`\n${renderToc(findHeaders(parseResult), toc.label)}` : ""
}
<div id="observablehq-center">${renderHeader(options, parseResult.data)}
Expand All @@ -99,8 +100,15 @@ ${html.unsafe(parseResult.html)}</main>${renderFooter(path, options, parseResult
`);
}

async function renderSidebar(title = "Home", pages: (Page | Section)[], path: string): Promise<Html> {
return html`<input id="observablehq-sidebar-toggle" type="checkbox" title="Toggle sidebar">
async function renderSidebar(
title = "Home",
pages: (Page | Section)[],
path: string,
sidebar: true | "hidden"
): Promise<Html> {
return html`<input id="observablehq-sidebar-toggle" type="checkbox" title="Toggle sidebar"${
sidebar === "hidden" ? " data-hidden=1" : ""
Fil marked this conversation as resolved.
Show resolved Hide resolved
}>
<label id="observablehq-sidebar-backdrop" for="observablehq-sidebar-toggle"></label>
<nav id="observablehq-sidebar">
<ol>
Expand Down Expand Up @@ -262,3 +270,15 @@ function renderPager(path: string, {prev, next}: PageLink): Html {
function renderRel(path: string, page: Page, rel: "prev" | "next"): Html {
return html`<a rel="${rel}" href="${relativeUrl(path, prettyPath(page.path))}"><span>${page.name}</span></a>`;
}

// Validates the specified required string against the allowed list of keywords.
function keyword(input: any, name: string, allowed: string[]): string {
const i = `${input}`.toLowerCase();
if (!allowed.includes(i)) throw new Error(`invalid ${name}: ${input}`);
return i;
}

function maybeSidebar(sidebar: string | boolean, pages): "hidden" | boolean {
if (sidebar === "auto") sidebar = pages.length > 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point sidebar isn’t normalized, so the "auto" check will miss e.g. "AUTO".

return typeof sidebar === "boolean" ? sidebar : (keyword(sidebar, "sidebar", ["hidden"]) as "hidden");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw an error on sidebar: null, but I think we want to coerce any non-string value to a boolean. Something like:

Suggested change
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.

}
2 changes: 2 additions & 0 deletions test/config-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe("readConfig(undefined, root)", () => {
root: "test/input/build/config",
output: "dist",
style: {theme: ["air", "near-midnight"]},
sidebar: true,
pages: [
{path: "/index", name: "Index"},
{path: "/one", name: "One<Two"},
Expand All @@ -35,6 +36,7 @@ describe("readConfig(undefined, root)", () => {
root: "test/input/build/simple",
output: "dist",
style: {theme: ["air", "near-midnight"]},
sidebar: true,
pages: [{name: "Build test case", path: "/simple"}],
title: undefined,
toc: {label: "Contents", show: true},
Expand Down