docs: correct the user docs against what the theme actually does - #229
Conversation
Six documented behaviours the theme does not have, found by the #147 review and verified against the code here: - the toolbar has no site title, and the Home link, the font-size control and the GitHub link went unmentioned; - the "On this page" outline appears from 1280px, not below `lg` (1024px): `.simple-center-grid > .col-margin` is `hidden xl:block`; - `#222` is this theme's dark ground, but it was the book theme's before 0.22.0, and the release the lecture repositories pin has a navy-charcoal palette. Which palette to use is an open design question, so the page no longer presents the two as matching; - `site.title` feeds `og:site_name` and the page-title suffix; the header shows the project title; - nothing limits an output's height, so a long output renders at full length -- the Sphinx scroll-output cell tags have no effect yet; - only `translators_label` is translatable: the names themselves are joined in English on every edition, right-to-left ones included. `docs/index.md` now points its no-counterpart promise at the migration page's table, and no longer presents PLAN.md as the live record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZLpDVYu1YBZHQfwkRRJj7
|
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
There was a problem hiding this comment.
🟡 Changes recommended
The docs/layout.md Header section incorrectly claims the full-screen and font-size controls move into the mobile overflow menu, when the code hides them below md and only downloads/launch/GitHub move to the overflow.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This documentation-only PR (addressing #217) corrects several docs/ pages so they describe what the theme actually does rather than legacy/assumed behaviour. I verified each corrected claim against the code, and all but one hold up.
Changes:
- Rewrites the
docs/layout.mdHeader and Widths sections (toolbar contents, no site title, outline appears from 1280px) and correctsdocs/dark-mode.md,docs/configuration.md,docs/notebooks.md, anddocs/authors.md. - Repoints
docs/index.md's "no-counterpart" promise at the migration page's table and drops thePLAN.mdreference. - Adds a
### FixedCHANGELOG entry under[Unreleased].
File summaries
| File | Description |
|---|---|
| docs/layout.md | Rewrites Header (toolbar controls, no site title, overflow menu) and Widths (1280px outline breakpoint) — overflow-menu sentence is inaccurate |
| docs/dark-mode.md | Reframes #222 as the theme's own ground with palette provenance and open design question (#187) — matches qepage-dark |
| docs/configuration.md | Corrects site.title to feed og:site_name + title suffix, not the header — matches seo.ts/ProjectFrontmatter |
| docs/notebooks.md | States nothing limits output height and images centre while tables stay left — no output max-height exists |
| docs/authors.md | Notes translator names are joined in English on every edition — matches PeopleList |
| docs/index.md | Points no-counterpart promise at migrating.md (file added by #226) |
| CHANGELOG.md | Adds ### Fixed entry; missing the customary PR link |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| not the header), and notebook output height (nothing clips a long output). | ||
| `docs/authors.md` now says the translator names are joined in English on | ||
| every edition | ||
| ([#217](https://github.com/QuantEcon/quantecon-theme.mystmd/issues/217)). |
There was a problem hiding this comment.
Added the PR link in 043b92a — the entry now ends ([#217](…/issues/217)) ([#229](…/pull/229)).
Worth saying that this one is argued accurately, which has not always been true of this comment on other PRs: scoping the claim to the [2.7.0] section is exactly right. I counted that section — 10 entries, all 10 ending with a pull/ link, none issue-only. Across the whole file the practice is mixed (ten entries carry only an issue link, spread over five released versions), so 2.7.0 is the defensible frame to cite, and CONTRIBUTING.md:227 settles it regardless.
That same commit merges main, which gained seven [Unreleased] entries while this was open. The ### Fixed entry here folds into the Fixed section main already had, rather than opening a second one.
#221, #222, #224, #225, #226, #227 and #228 all landed `[Unreleased]` entries while this was open. This branch adds a `### Fixed` entry, so it folds into main's existing Fixed section (output image centring, base-URL head links) rather than opening a second one, and it gains its PR link beside the issue link per CONTRIBUTING.md:225-227. `docs/configuration.md`, `docs/index.md` and `docs/layout.md` auto-merged, and each was checked rather than assumed: layout.md holds this branch's Header and Widths rewrites beside #228's h2-to-h4 outline text and #221's default-footer section; index.md holds #226's Pages-table row beside this branch's rewritten closing paragraph; configuration.md holds the rows from all four PRs that touched its table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Header section said the full-screen toggle, the font-size control, downloads, launch and GitHub all move into an overflow menu below `md`. `MobileActionsMenu` renders only `DownloadsButton`, `LaunchButton` and `GitHubButton`; the full-screen toggle and `FontScaleListItems` are `hidden md:block` in `Toolbar.tsx` with no overflow counterpart. So all five leave the toolbar at that width, but only three reappear -- the other two are simply unavailable, which is what the section now says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses #217. Documentation only.
What I verified, and corrected
I checked every line in the issue's table against the code rather than taking the table's word for it. All six were wrong; here is what each actually is:
docs/layout.mdheaderdocs/layout.mdwidthslg(1024px).simple-center-grid > .col-marginishidden xl:block, so 1280pxdocs/dark-mode.md#222)"#222is this theme's ground (qepage-dark), but it was the book theme's before 0.18.0; the 0.22.0 the lecture repos pin is navy-charcoal. Now stated as the theme's own, with the palette named as an open design question (#187)docs/configuration.mdsite.titleis "the site name in the header"og:site_nameand the page-title suffix; the header shows the project titledocs/notebooks.mdmax-heighton any output rule. Now says so, and names QuantEcon/mystmd#106docs/authors.mdtranslators_labelis the only translatable part — the names are joined in English (", "/" and ") on every edition, RTL includeddocs/index.mdnow points its no-counterpart promise at the migration page's table and no longer presentsPLAN.mdas the live record.Two criteria I could not satisfy, and why
I'd rather flag these than pretend:
vX.Y.ZURLs name the release that closes the gate." That release does not exist — it is cut after these PRs merge. The URLs still say v2.7.0. This needs a follow-up commit once you know the version, or folding into the release-prep PR.main:docs/layout.mddocuments the default footerdocs/migrating.md"Lecture sources" names the rewrite scriptdocs/launch.md,docs/configuration.mdand README namelaunch_notebook_repo/launch_colabdocs/configuration.mdand README describe the canonical linkdocs/layout.mddocuments h4 in the outlinedocs/migrating.mdexists and is indexedSo six of the seven landed with their own PR, and the seventh was a real gap: #225 shipped output-image centring with no documentation at all, and
docs/notebooks.mdhere is what closes it. That is worth knowing rather than assuming each PR documented itself.So this PR is the docs review's substance, not its sign-off.
Merged with main
Seven PRs landed
[Unreleased]entries while this was open. This branch adds a### Fixedentry, so it folds into the Fixed section main already had (output image centring, base-URL head links) rather than opening a second one. Merged at043b92ada.docs/configuration.md,docs/index.mdanddocs/layout.mdauto-merged, and each was verified rather than assumed:layout.mdholds this branch's Header and Widths rewrites beside #228's h2-to-h4 outline text and #221's default-footer section;index.mdholds #226's Pages-table row beside the closing paragraph this PR owns;configuration.mdholds the rows from all four PRs that touched its table.Review
Copilot raised two comments, both addressed.
MobileActionsMenurenders onlyDownloadsButton,LaunchButtonandGitHubButton, while the full-screen toggle (Toolbar.tsx:66-67) andFontScaleListItems(69) arehidden md:blockwith no overflow counterpart — so all five controls leave the toolbar belowmd, but only three reappear. Corrected in788d44ffd.043b92ada). That comment scoped its claim to the[2.7.0]section, which is accurate: 10 entries there, all 10 ending with apull/link.The other five corrections in this PR were re-checked independently against the code and all hold: no site title in the toolbar; the outline appearing from 1280px (
hidden xl:block);site.titlefeedingog:site_nameand the title suffix while the header usesprojectTitle; nothing limiting output height (the onlymax-heightinquantecon.cssbelongs to.qe-outline, not an output rule); and translator names joined in English on every edition (PeopleListhardcodes' and 'and', 'with no locale argument).Ordering
Several of these touch files the other gate PRs also touch:
docs/configuration.md— feat(launch): make the Launch control opt-in and explicitly configured #224 edits the launch rows and theproject.githubrow, feat(seo): emit a canonical link, and base-URL-aware head links #227 thesite_urlrow, docs: add a migration checklist for repos moving off the Sphinx theme #226 adds rows to the same "Othersitekeys" table. This PR edits thesite.titlerow in that table, so expect a small conflict with whichever lands adjacent.docs/layout.md— feat(outline): list h4 subsections, and expand at every depth #228 rewrites the "On this page" section; this PR rewrites Header and Widths. Different sections.docs/index.md— I found docs: add a migration checklist for repos moving off the Sphinx theme #226 and this PR both rewriting the same closing paragraph, so I removed that hunk from docs: add a migration checklist for repos moving off the Sphinx theme #226, leaving it only the Pages-table row its own criteria name. This PR now owns that paragraph outright.Merge before #218. The code PRs it had to follow — #221, #222, #224, #225, #226, #227 and #228 — have all merged, and main is merged in here (see below), so that ordering constraint is satisfied.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UZLpDVYu1YBZHQfwkRRJj7
Generated by Claude Code