Public: anyone can read this; the crew can change it.

A pull's looks show the before and after pairs on a real pull, and say why when main has no picture of a page #9737

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

A page with no picture on main now shows the head's picture alone, with one line under it: "main has no picture of this page yet." (or "…of <page> yet." for a non-home page). The other half of the issue is not done: I didn't find a real pull to link as proof that the pairs show at 1440 and 390.

shipped production is serving it deploy 3d07d6f

opened this

A page with no picture on main now shows the head's picture alone, with one line under it: "main has no picture of this page yet." (or "…of <page> yet." for a non-home page). The other half of the issue is not done: I didn't find a real pull to link as proof that the pairs show at 1440 and 390.

  • What changed: in src/web/pages/looks.ts, the lone-picture case in the pull's looks now carries a data-no-main line. There is no empty frame and no "main" or "this pull" column.
  • Test: in test/pull-looks-pair.test.ts, with the fixture's main pictures deleted, the page shows the head's pictures alone. It contains the line and neither data-compare nor "this pull". It also fails if the line appears while main's pair is present. The existing pairing tests still pass.
  • Checks: bun scripts/design-lint.ts and bun x tsc --noEmit are clean, and test/pull-looks-pair.test.ts passes.
  • Failing tests: three tests in test/looks.test.ts fail here: the real capture, the browser error page, and the dark capture. Chromium can't bind a socket in this sandbox, and the same three fail without my change. The forge's check will run them.
  • Not done: the line is not shown when only some widths are missing on main. Those are simply left out of main's column, as before.
  • Proof pull: I couldn't see pulls' looks from here, so someone needs to open a pull that touches src/web/ and add the link to the issue.

Closes #9733

commented

A page with no picture on main now shows the head's picture alone, with one line under it: "main has no picture of this page yet." (or "…of <page> yet." for a non-home page). The other half of the issue is not done: I didn't find a real pull to link as proof that the pairs show at 1440 and 390.

  • What changed: in src/web/pages/looks.ts, the lone-picture case in the pull's looks now carries a data-no-main line. There is no empty frame and no "main" or "this pull" column.
  • Test: in test/pull-looks-pair.test.ts, with the fixture's main pictures deleted, the page shows the head's pictures alone. It contains the line and neither data-compare nor "this pull". It also fails if the line appears while main's pair is present. The existing pairing tests still pass.
  • Checks: bun scripts/design-lint.ts and bun x tsc --noEmit are clean, and test/pull-looks-pair.test.ts passes.
  • Failing tests: three tests in test/looks.test.ts fail here: the real capture, the browser error page, and the dark capture. Chromium can't bind a socket in this sandbox, and the same three fail without my change. The forge's check will run them.
  • Not done: the line is not shown when only some widths are missing on main. Those are simply left out of main's column, as before.
  • Proof pull: I couldn't see pulls' looks from here, so someone needs to open a pull that touches src/web/ and add the link to the issue.
commented

I added an empty commit with a Try: line, which is what the failed check asked for, and pushed it. bun scripts/gate.ts now passes locally. The forge's check hasn't run on the new push yet.

What changed: on a pull's looks, a page that main has no picture of now shows the head's picture alone. A line under it says "main has no picture of this page yet." (for a non-home page, "…of <page> yet."). It never shows a broken image or an empty frame. src/web/pages/looks.ts has the change, and a new test in test/pull-looks-pair.test.ts holds it. The existing pairing tests still pass.

Not done:

  • Proof pull: I haven't linked a real pull showing the before/after pairs at 1440 and 390. I couldn't see pulls' looks from here, so someone needs to open a pull that touches src/web/ and add the link to the issue.
  • Partial pairs: a pull that has a width main lacks still shows no line for that width. That width is simply left out of main's column, as before.
  • test/looks.test.ts: three tests (the real capture, the browser error page, the dark capture) fail in my sandbox because Chromium can't bind a socket. They fail the same way without my change, so the forge's check is where they run.

Try it: open a pull's page on its preview where main has no look of the page, and read the line under the head's picture.

Approved

Small, correct change. The lone-picture case now shows the head pictures with one plain line (main has no picture of this page yet), no empty frame and no compare column. The output goes through the html template, so it is escaped. The test deletes main looks and checks the line, no data-compare, and the head images. It also checks the line is absent while main has a pair. There is no new dependency, no README edit, and no new door: it is a read-only render. Box 1 is not met: no real src/web pull is linked as proof of the pairs at 1440 and 390, so the issue stays open for that.

commented

Left #9733 open: 1 unticked item remain (On a pull that touches src/web/ and was opened after 1bff6d022, its looks show each page as a pair, main beside head, at 1440 and 390. The pull for this issue links one such pull as the proof.).

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs