fix(notebooks): centre images in cell outputs - #225
Conversation
A plot produced by a lecture's code cells sat against the left edge of the content column with all the spare width beside it: 0px to its left and 232px to its right, for a 568px figure in the 800px column. The lecture builds centre the same plots. A plot output is a bare `<img>` that upstream renders with neither class nor style, inside a container marked only by `data-name`, so the container attribute is what there is to select on; `text-align` on the container cannot move it, because Preflight makes the image a block. Auto side margins on the image do, matching the lecture builds' own `.cell_output img` rule. Both container names are covered: the stored outputs are re-rendered into `active-outputs-container` when a reader starts live compute, and the plot should not jump left at that moment. Only images are centred, so tables and text outputs stay left-aligned as they are on the lecture sites. The fixture notebook gains a cell with a stored `image/png` output -- wider than the mobile column and narrower than the desktop one, so the same page exercises both centring and capping -- and a DOM assertion measures the gaps. The pixel suite alone could not catch this: the plot is mostly white, so the shift moves 0.36% of the page, under the suite's 1% tolerance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZLpDVYu1YBZHQfwkRRJj7
|
/update-snapshots
The Generated by Claude Code |
|
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
|
🎭 Refreshed visual baselines in 6dd380c:
|
There was a problem hiding this comment.
🟡 Changes recommended
The changelog entry should include a PR link per CONTRIBUTING, and the new overflow assertion can be made more robust to avoid platform-dependent test flakiness.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR aligns notebook cell image outputs with QuantEcon lecture-site behavior by centering output <img> elements within the outputs container, including the live-compute “active outputs” container, and adds a visual/DOM assertion plus fixture data to prevent regressions.
Changes:
- Center notebook output images via a scoped CSS rule targeting
data-name="outputs-container"/data-name="active-outputs-container". - Extend the visual test suite with a DOM-based centering/overflow assertion and update the notebook fixture to include a stored PNG output.
- Document the fix in
CHANGELOG.mdunder Unreleased → Fixed.
File summaries
| File | Description |
|---|---|
| styles/quantecon.css | Adds a base-layer rule to center images inside notebook outputs containers. |
| tests/visual/theme.spec.ts | Adds a test asserting desktop centering and no horizontal overflow, plus a guard that text outputs remain left-aligned. |
| tests/visual/fixture/notebook.ipynb | Adds a stored image/png output cell to exercise centering behavior. |
| CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the centering change. |
Review details
- Files reviewed: 4/6 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
#221, #222 and #224 all landed entries under `## [Unreleased]` while this was open, which conflicted textually with this branch's `### Fixed` entry while agreeing in substance. Resolved by keeping all four, in the order Keep a Changelog uses: Added (default site footer, `{raw}` rewriter), Changed (breaking launch change), then Fixed (output image centring). The Fixed entry also gains its PR link alongside the issue link, which is what CONTRIBUTING.md asks for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The overflow check was a strict boolean on `scrollWidth > clientWidth`. Both values are integers rounded from sub-pixel layout, so a page that fits exactly can still report one more pixel of scrollWidth than clientWidth -- and `expect(false).toBe(false)` names neither value when it fails. It now compares them with the same 1px allowance as the gap assertions beside it, and returns the two widths so a failure reports them. Page overflow itself is guarded by the 1280/1300/1328px sweep across both text directions in `outline-within-viewport`; this line is a sanity check that the page holding a centred image still fits, which is why it keeps `clientWidth` as its subject rather than the viewport width -- the latter includes the scrollbar and would tolerate roughly 15px more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fixture notebook gains a stored `image/png` cell, so /notebook grows from 1094 to 1551px on desktop and from 1203 to 1548px on mobile. Both darwin baselines were size mismatches against that and failed outright, so the default update mode was sufficient: it rewrites only a baseline whose comparison fails, which is why the other twelve darwin baselines are untouched. (A sub-threshold change needs `=all` instead, as the rtl baseline did in #224.) The heights land a few pixels off the linux set's 1552/1553, which is the reason these baselines are platform-suffixed at all -- the CI font stack wraps slightly differently, so neither platform's pixels can satisfy the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#221, #222, #224 and #225 all landed entries under `## [Unreleased]` while this was open. This branch's entry is `### Added` like two of theirs, so it folds into that list rather than forming a second section: Added (default site footer, `{raw}` rewriter, this checklist), then main's Changed and Fixed sections unchanged. The entry also gains its PR link alongside the issue link, which is what CONTRIBUTING.md:225-227 asks for. `docs/configuration.md` auto-merged, and was checked rather than assumed: main's launch rows from #224 and this branch's `project.description`, `project.keywords` and `project.math` rows are all present, with no row duplicated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#221, #222, #224, #225 and #226 all landed entries under `## [Unreleased]` while this was open. This branch adds one `### Added` entry and one `### Fixed` entry, so each folds into the matching section rather than opening a second: Added (default site footer, `{raw}` rewriter, migration checklist, this canonical link), Changed (the breaking launch change), Fixed (output image centring, these base-URL head links). Both entries also gain their PR link beside the issue link, which is what CONTRIBUTING.md:225-227 asks for. `README.md`, `docs/configuration.md`, `template.yml` and `tests/visual/theme.spec.ts` auto-merged. Each was checked rather than assumed: both sides' content is present in all four. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#221, #222, #224, #225, #226 and #227 all landed entries under `## [Unreleased]` while this was open. This branch adds one `### Added` entry, so it folds into that list rather than opening a second section, and it gains its PR link beside the issue link, which is what CONTRIBUTING.md:225-227 asks for. `docs/layout.md`, `styles/quantecon.css` and `tests/visual/theme.spec.ts` auto-merged. Each was checked rather than assumed: the branch's h4 indent rule sits beside main's output-image centring rule, the branch's outline assertions beside main's footer, launch, centring and canonical tests, and the branch's h2-to-h4 documentation beside main's default-footer section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#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>
Closes #206, implementing the 2026-09-11 decision (images only; tables and text stay left-aligned; both container names; no upstream-registry entry).
Problem, re-verified on main @ d7ec6cd
Added the fixture's image cell, built main, and measured in Chromium at 1280px: the 568px plot sits at
left: 0, right: 232in its 800px outputs container — reproducing the issue's measurement of the lecture-python-programming preview exactly. After the change:left: 116, right: 116.Change
One rule in
styles/quantecon.css's@layer baseblock:A plot output is a bare
<img>that upstream renders with neither class nor style, inside a container marked only bydata-name— so the container attribute is what there is to select on, and no renderer override is needed.text-alignon the container cannot move it, because Preflight makes the image a block. Only side margins are set, so vertical rhythm is untouched, and only images, so tables and text outputs stay left-aligned.Live compute is covered. The stored outputs are re-rendered into
active-outputs-containerwhen a reader starts compute. Rather than boot Pyodide, I renamed the container'sdata-nameon a live page to what upstream uses in that state and re-measured: still116 / 116. The rule also survives Tailwind's content scan (it names no class, so it is always emitted) — confirmed present in the builtapp.css.Tests
output-image-centredintheme.spec.ts, on the thebe-enabled fixture whosenotebook.ipynbnow carries a storedimage/pngcell (568×320 — wider than the mobile column, narrower than the desktop one, so one page exercises both centring and capping):Mutation-tested: with the rule's body emptied, the assertion fails; restored, it passes. (After #205, where my first "no empty slot" assertion turned out to be unfalsifiable, I'm checking that every new assertion can actually fail.)
Visual baselines —
notebook.pngmust be refreshed on both projectsThe fixture notebook gains a cell, so the page grows 1095px → 1552px on desktop. That is a size mismatch, so
notebook.pngfails outright until refreshed.Worth recording, because it is the reason the DOM assertion exists: the centring itself would not have been caught by the pixel suite. I captured full-page renders from a clean build with and without the rule and diffed them directly:
A plot is mostly white, so shifting it moves very few pixels.
Nothing else moves: the rule only matches images in cell outputs, and only
/notebookhas one.intro,features,lists,history-open,sidebar-openandrtlare untouched, as the issue's criteria require.-linux— already refreshed in6dd380c28, and nothing further is needed: theVisual regression (Chromium)job passes on the merged tip, so those baselines are correct against main as it now stands. I had planned to re-trigger/update-snapshotsafter merging main and deliberately held it until CI could answer, rather than spend a refresh commit (whose own CI arrives gated) on a set that turned out to be current.-darwin— refreshed locally in78c53c3da. Both files were hard size mismatches (desktop 1094 → 1551, mobile 1203 → 1548), so the default update mode was the right tool: it rewrites only a baseline whose comparison actually fails, leaving the other twelve darwin baselines untouched. (A sub-threshold change is the case that needs=allinstead, as the rtl baseline did in feat(launch): make the Launch control opt-in and explicitly configured #224.) Verified with a clean non-updating re-run: 44 passed, 0 failed. The heights land a few pixels off the linux set's 1552/1553, which is precisely why these baselines are platform-suffixed — neither platform's pixels can satisfy the other.Merged with main
#221, #222 and #224 all landed while this was open, and all three added entries under
## [Unreleased], which conflicted textually with this branch's### Fixedentry while agreeing in substance. Merged main in at153d1422d, keeping all four in the order Keep a Changelog uses: Added (default site footer,{raw}rewriter), Changed (breaking launch change), then Fixed (this centring fix).Checked rather than assumed, since a clean text merge is no guarantee the content survived: the CSS rule is present in source and in the built bundle (
public/build/_assets/app-*.css, so Tailwind's content scan did not drop it), the fixture still carries its storedimage/pngcell whilefixture-no-thebestill carries none,tscis clean, and the unit suite is unchanged. A full local run against the merged tree failed on exactly two baselines — the darwinnotebook.pngpair — and nothing else drifted, withoutput-image-centredpassing on both projects.Review
Copilot raised two comments, both addressed and answered:
theme.spec.ts) — changed in9343d3b5b, though not in the shape suggested. It now compares the widths numerically with the same 1px allowance the gap assertions beside it use, and returns both values so a failure names them. I keptdocumentElement.clientWidthas the subject rather than the viewport width used inoutline-within-viewport:clientWidthexcludes the scrollbar, so adopting that comparison would have made the assertion roughly 15px laxer rather than merely more stable. The cost is stated plainly in the reply — with a tolerance, a genuine 1px overflow now passes here, which is acceptable only because page overflow is really guarded by that 1280/1300/1328px sweep across both text directions.153d1422d. Its supporting claim was fair this time ("most existing entries include both"), unlike the same comment on feat(scripts): rewrite Sphinx{raw}blocks out of lecture sources #222 and feat(launch): make the Launch control opt-in and explicitly configured #224; for the record the exceptions are ten issue-only entries across five released versions.A methodology note that may be worth a follow-up
Two traps cost me real time here, and both could silently mislead anyone doing an A/B build comparison in this repo:
make build-themenever cleans.remix builddoes not clearbuild/, sobuild/index.jsfrom a previous build leaked into my "baseline" bundle and made it render as the candidate.npm run cleanfirst fixes it — but nothing in the Makefile or CONTRIBUTING says so.serve.shinstances on the sameFIXTURE_DIRrace.serve.shwritesmyst.ymlinto the shared fixture directory, so a second server rewrites the first one's config out from under it. They have to be run sequentially, or on separate fixture copies.Both are now filed, each verified against the code first rather than taken on report: #233 for the missing clean step (the target removes only
.deploy/<theme>, and neither it norprod:buildrunsnpm run clean, while Makefile lines 21-22 copypublic/andbuild/wholesale into the bundle), and #234 for theserve.shrace (myst.ymlis generated inside the fixture directory it serves, so two instances on oneFIXTURE_DIRoverwrite each other — the committed suite is safe, because its three servers use three different fixture directories).🤖 Generated with Claude Code
https://claude.ai/code/session_01UZLpDVYu1YBZHQfwkRRJj7
Generated by Claude Code