[PR #1514] [MERGED] fix(shared-sidebar): tighten vertical spacing for uniform Quarto look #9180

New Issue

GiteaMirror · 2026-05-03T01:25:11-05:00

GiteaMirror commented

2026-05-03 01:25:11 -05:00

📋 Pull Request Information

Original PR: https://github.com/harvard-edge/cs249r_book/pull/1514
Author: @profvjreddi
Created: 4/24/2026
Status: ✅ Merged
Merged: 4/24/2026
Merged by: @profvjreddi

Base: dev ← Head: fix/shared-sidebar-tighten-spacing

📝 Commits (1)

61dae63 fix(shared-sidebar): tighten vertical spacing to match book/kits/labs/mlsysim

📊 Changes

1 file changed (+20 additions, -14 deletions)

View changed files

📝 shared/styles/partials/_sidebar.scss (+20 -14)

📄 Description

Summary

Unifies sidebar vertical rhythm across all Quarto sites. Before this PR, the shared partial was loose (padding `4px 8px`, margin `1px 0`, explicit line-height `1.45`, extra padding and `margin-top` on section headers), while book/kits/labs/mlsysim already shipped their own tight local override (padding `2px 6px`, margin `0.5px 0`). Net effect: TinyTorch and the landing site looked noticeably airier than the book.

The comment was also wrong

The partial's own file header rationalized "more air than CSS-reset minimums to improve scanning" as a deliberate choice. That was horizontal-vs-vertical-spacing confusion:

Horizontal 6–8px padding DOES help mouse/touch targeting → keep
Vertical 4px padding + explicit margin-top on section headers makes a 60-item sidebar require more scrolling → remove

The book has the most items per sidebar in the ecosystem and it runs the tight form. If it's right there, it's right everywhere.

What changes

Selector	Before (loose)	After (tight)
`.sidebar-item a` padding	`4px 8px`	`2px 6px`
`.sidebar-item a` margin	`1px 0`	`0.5px 0`
`.sidebar-item a` line-height	`1.45` (explicit)	removed (Bootstrap default ~1.5)
Section header padding	`6px 8px`	removed (inherits from item rule)
Section header margin-top	`4px`	removed (no extra gap above sections)

Sites affected

TinyTorch, landing site: now tight (they import the shared partial directly) ✓
book, kits, labs, mlsysim: no visual change (their local overrides already specify these same tight values and take precedence)
slides, instructors: no sidebar so N/A

Follow-up (not in this PR)

Four sites still carry duplicate local copies of these rules. Each passes `${site}-accent` explicitly instead of the shared partial's `$accent` variable, which is why the duplicates exist. A follow-up could either:

Confirm each site defines `$accent` before the partial and delete the four local overrides → true single source of truth.
Or keep the per-site overrides but shrink them to "just the accent color" and inherit spacing from the partial.

Either option is a structural cleanup, not a visual change. Deferring until we have eyes on the deployed preview of this PR.

Test plan

`_sidebar.scss` scss syntax still parses (no local SCSS build, relies on downstream Quarto builds)
Visual audit in repo: confirmed TinyTorch imports this partial, confirmed book/kits/labs/mlsysim all ship local overrides that MATCH the new tight values
Post-merge: preview deploy on TinyTorch and eyeball the sidebar against book's sidebar side-by-side

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/harvard-edge/cs249r_book/pull/1514 **Author:** [@profvjreddi](https://github.com/profvjreddi) **Created:** 4/24/2026 **Status:** ✅ Merged **Merged:** 4/24/2026 **Merged by:** [@profvjreddi](https://github.com/profvjreddi) **Base:** `dev` ← **Head:** `fix/shared-sidebar-tighten-spacing` --- ### 📝 Commits (1) - [`61dae63`](https://github.com/harvard-edge/cs249r_book/commit/61dae6332d7a47ce89be44417c8fe7edde1f815c) fix(shared-sidebar): tighten vertical spacing to match book/kits/labs/mlsysim ### 📊 Changes **1 file changed** (+20 additions, -14 deletions) <details> <summary>View changed files</summary> 📝 `shared/styles/partials/_sidebar.scss` (+20 -14) </details> ### 📄 Description ## Summary Unifies sidebar vertical rhythm across all Quarto sites. Before this PR, the shared partial was loose (padding \`4px 8px\`, margin \`1px 0\`, explicit line-height \`1.45\`, extra padding and \`margin-top\` on section headers), while book/kits/labs/mlsysim already shipped their own tight local override (padding \`2px 6px\`, margin \`0.5px 0\`). Net effect: TinyTorch and the landing site looked noticeably airier than the book. ## The comment was also wrong The partial's own file header rationalized "more air than CSS-reset minimums to improve scanning" as a deliberate choice. That was horizontal-vs-vertical-spacing confusion: - Horizontal 6–8px padding DOES help mouse/touch targeting → keep - Vertical 4px padding + explicit margin-top on section headers makes a 60-item sidebar require more scrolling → remove The book has the most items per sidebar in the ecosystem and it runs the tight form. If it's right there, it's right everywhere. ## What changes | Selector | Before (loose) | After (tight) | |---|---|---| | \`.sidebar-item a\` padding | \`4px 8px\` | \`2px 6px\` | | \`.sidebar-item a\` margin | \`1px 0\` | \`0.5px 0\` | | \`.sidebar-item a\` line-height | \`1.45\` (explicit) | removed (Bootstrap default ~1.5) | | Section header padding | \`6px 8px\` | removed (inherits from item rule) | | Section header margin-top | \`4px\` | removed (no extra gap above sections) | ## Sites affected - **TinyTorch, landing site**: now tight (they import the shared partial directly) ✓ - **book, kits, labs, mlsysim**: no visual change (their local overrides already specify these same tight values and take precedence) - **slides, instructors**: no sidebar so N/A ## Follow-up (not in this PR) Four sites still carry duplicate local copies of these rules. Each passes \`\$\{site\}-accent\` explicitly instead of the shared partial's \`\$accent\` variable, which is why the duplicates exist. A follow-up could either: 1. Confirm each site defines \`\$accent\` before the partial and delete the four local overrides → true single source of truth. 2. Or keep the per-site overrides but shrink them to "just the accent color" and inherit spacing from the partial. Either option is a structural cleanup, not a visual change. Deferring until we have eyes on the deployed preview of this PR. ## Test plan - [x] \`_sidebar.scss\` scss syntax still parses (no local SCSS build, relies on downstream Quarto builds) - [x] Visual audit in repo: confirmed TinyTorch imports this partial, confirmed book/kits/labs/mlsysim all ship local overrides that MATCH the new tight values - [ ] Post-merge: preview deploy on TinyTorch and eyeball the sidebar against book's sidebar side-by-side --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

GiteaMirror added the pull-request label 2026-05-03 01:25:11 -05:00

GiteaMirror closed this issue

2026-05-03 01:25:12 -05:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/cs249r_book#9180