Public: anyone can read this; the crew can change it.
The pull page speaks the home page's language, with looks before and after #9659
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9588-the-pull-page-speaks-the-home-page-s-lan → main 4 commits fork 7 threads · 7 unresolved
3 files
changed +83
−15
docs/design/areas/pull.md +2
| @@ -13,3 +13,5 @@ The terminal's pull screen (`src/tui/screens/pull.ts`) draws its own rows and sh | ||
| 13 | 13 | On a 390 px phone the page does not scroll sideways (#9327): the title (`.page-head h1`) and every `.mono` (branch names) wrap with `overflow-wrap: anywhere`, comments already do through `.prose`. The diff scrolls inside its own box. No new class; `test/pull-phone-fit.test.ts` holds it. |
| 14 | 14 | |
| 15 | 15 | `.split` (content.ts) has one `minmax(0, 1fr)` column below 960 px, so a long unbroken line in a comment cannot widen the page. It is global to every `.split` page; the `min-width: 960px` and `data-rail="wide"` rules come after it in the same file and still set their two columns, so desk layouts are unchanged. |
| 16 | ||
| 17 | The pull page's cards (merge box, review, commits, slices, run-this-code) are `.card.bare` (#9588): rows and a heading on the page's one background, no box. A bare card draws no tone, so the merge row carries none on the card: its state is the sentence's opening word ("Ready to merge", "Waiting on checks.", "Merged.", "Closed without merging.") with the tone's `.dot` before it, a word and a mark, not colour alone. The team verdicts are a `.list` of `.row`s, not a card. What stays a box: each file's diff (code scrolls inside its own frame) and the comments, which `thread.ts` draws for the issue page too; turning those into rows is the timeline's change, not this page's. No new class; `test/pull-merge-box.test.ts` holds it. |
src/web/pages/pull.ts +20 −11
| @@ -95,6 +95,7 @@ import { | ||
| 95 | 95 | badge, |
| 96 | 96 | btn, |
| 97 | 97 | card, |
| 98 | dot, | |
| 98 | 99 | field as formField, |
| 99 | 100 | icon, |
| 100 | 101 | hintAttr, |
| @@ -431,7 +432,12 @@ function originCheckLine(ctx: Ctx, repo: Repo, pull: Pull): Html | "" { | ||
| 431 | 432 | return html`<p class="muted">${originCheckLead(repo.origin.fullName, kept, pull.headOid)}, as of ${time(kept.checkedAt)}</p>`; |
| 432 | 433 | } |
| 433 | 434 | |
| 434 | /** One §5 row in a toned card; `.setting-list` wraps the sentence and drops the action under it on a phone. */ | |
| 435 | /** | |
| 436 | * One §5 row on a bare card; `.setting-list` wraps the sentence and drops the | |
| 437 | * action under it on a phone. A bare card draws no tone, so the state is the | |
| 438 | * sentence's opening word ("Merged.", "Ready to merge", "Waiting on checks.") | |
| 439 | * with the tone's dot beside it: a word, not colour alone (principle 6). | |
| 440 | */ | |
| 435 | 441 | function box( |
| 436 | 442 | tone: Tone | undefined, |
| 437 | 443 | lead: Html, |
| @@ -440,16 +446,17 @@ function box( | ||
| 440 | 446 | meta: Html, |
| 441 | 447 | trail: Html | null, |
| 442 | 448 | ): Html { |
| 449 | const said = typeof title === "string" ? raw(title) : title; | |
| 443 | 450 | return card( |
| 444 | 451 | html`<ul class="list setting-list"> |
| 445 | 452 | ${row({ |
| 446 | 453 | lead, |
| 447 | title: typeof title === "string" ? raw(title) : title, | |
| 454 | title: html`${dot(tone)} ${said}`, | |
| 448 | 455 | meta, |
| 449 | 456 | ...(trail ? { trail } : {}), |
| 450 | 457 | })} |
| 451 | 458 | </ul>`, |
outdated
Principle 1: the sentence already opens with the state ('Merged.', 'Blocked ...'), so the word says it twice. Drop the lead word from the sentences or reuse the sentence. Also |
||
| 452 | { ...(tone ? { tone } : {}), pad: false, extra: "mt" }, | |
| 459 | { pad: false, extra: "mt bare" }, | |
| 453 | 460 | ); |
| 454 | 461 | } |
| 455 | 462 | |
| @@ -709,17 +716,17 @@ const VERDICT_WORDS: Record<string, string> = { | ||
| 709 | 716 | /** One line per team the closed issue names: whose verdict it is, or that it is awaited (#2681). */ |
| 710 | 717 | export function teamsCard(verdicts: readonly TeamVerdict[]): Html { |
| 711 | 718 | if (verdicts.length === 0) return html``; |
| 712 | return html`<section class="card team-verdicts" aria-label="Team reviews"> | |
| 713 | <ul> | |
| 719 | return html`<section class="team-verdicts mt" aria-label="Team reviews"> | |
| 720 | <ul class="list"> | |
| 714 | 721 | ${verdicts.map( |
| 715 | (v) => html`<li> | |
| 722 | (v) => html`<li class="row"><span> | |
| 716 | 723 | <strong>${v.name}</strong>: |
outdated
Principle 7: a plain list is right, but |
||
| 717 | 724 | ${v.state === undefined |
| 718 | 725 | ? html`<span class="muted">awaiting a reviewer</span>` |
| 719 | 726 | : html`${VERDICT_WORDS[v.state] ?? v.state} by ${v.by ?? ""}${v.stale |
| 720 | 727 | ? html` <span class="muted">(on an older head)</span>` |
| 721 | 728 | : ""}`} |
| 722 | </li>`, | |
| 729 | </span></li>`, | |
| 723 | 730 | )} |
| 724 | 731 | </ul> |
| 725 | 732 | </section>`; |
| @@ -744,7 +751,7 @@ function reviewCard(ctx: Ctx, repo: Repo, pull: Pull, action: string): Html { | ||
| 744 | 751 | const user = ctx.auth.user; |
| 745 | 752 | if (user === undefined) |
| 746 | 753 | return card(html`<a href="/signin">Sign in</a> to review this change.`, { |
| 747 | extra: "mt", | |
| 754 | extra: "mt bare", | |
| 748 | 755 | head: html`${icon("eye")} |
| 749 | 756 | <h2>Review</h2>`, |
| 750 | 757 | }); |
| @@ -829,7 +836,7 @@ function reviewCard(ctx: Ctx, repo: Repo, pull: Pull, action: string): Html { | ||
| 829 | 836 | </div> |
| 830 | 837 | </form>`, |
| 831 | 838 | { |
| 832 | extra: "mt review-card", | |
| 839 | extra: "mt review-card bare", | |
| 833 | 840 | head: html`${icon("eye")} |
| 834 | 841 | <h2>Review</h2>`, |
| 835 | 842 | }, |
| @@ -1011,6 +1018,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> { | ||
| 1011 | 1018 | )} |
| 1012 | 1019 | </ul>`, |
| 1013 | 1020 | { |
| 1021 | extra: "bare", | |
| 1014 | 1022 | pad: false, |
| 1015 | 1023 | head: html`${icon("commit")} |
| 1016 | 1024 | <h2>${commits.length} commit${commits.length === 1 ? "" : "s"}</h2>`, |
| @@ -1279,7 +1287,7 @@ export function approveHereCard( | ||
| 1279 | 1287 | ${btn(proposed ? "Let it run here" : "Approve to run here", { size: "sm", icon: "check" })} |
| 1280 | 1288 | </form>`, |
| 1281 | 1289 | { |
| 1282 | extra: "mb", | |
| 1290 | extra: "mb bare", | |
| 1283 | 1291 | head: html`${icon("eye")} |
| 1284 | 1292 | <h2>Run this code?</h2>`, |
| 1285 | 1293 | }, |
| @@ -1506,7 +1514,7 @@ export async function pullSliceSubmit(ctx: Ctx): Promise<Response> { | ||
| 1506 | 1514 | * left, and — for a writer — a form to land the author's commits up to one |
| 1507 | 1515 | * of them as their own merge commit. |
| 1508 | 1516 | */ |
| 1509 | function slicesCard( | |
| 1517 | export function slicesCard( | |
| 1510 | 1518 | base: string, |
| 1511 | 1519 | thread: string, |
| 1512 | 1520 | slices: readonly LandedSlice[], |
| @@ -1553,6 +1561,7 @@ function slicesCard( | ||
| 1553 | 1561 | </form>`, |
| 1554 | 1562 | )}`, |
| 1555 | 1563 | { |
| 1564 | extra: "bare", | |
| 1556 | 1565 | head: html`${icon("merge")} |
| 1557 | 1566 | <h2>Slices</h2>`, |
| 1558 | 1567 | }, |
test/pull-merge-box.test.ts +61 −4
| @@ -12,6 +12,8 @@ import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | ||
| 12 | 12 | import { createSession } from "../src/core/sessions.ts"; |
| 13 | 13 | import { createUser, type User } from "../src/core/users.ts"; |
| 14 | 14 | import { serve, type Servers } from "../src/http/server.ts"; |
| 15 | import type { TeamVerdict } from "../src/core/issue_teams.ts"; | |
| 16 | import { teamsCard } from "../src/web/pages/pull.ts"; | |
| 15 | 17 | import { quietLogs } from "./helpers/globals.ts"; |
| 16 | 18 | |
| 17 | 19 | quietLogs(); |
| @@ -23,14 +25,26 @@ let alice: User; | ||
| 23 | 25 | const numbers: Record<string, number> = {}; |
| 24 | 26 | const cells = process.env["FORGE_CELLS"]; |
| 25 | 27 | |
| 26 | const boxOf = async (repo: string): Promise<string> => { | |
| 28 | const pageOf = async (repo: string): Promise<string> => { | |
| 27 | 29 | const res = await fetch( |
| 28 | 30 | `${servers.url}/alice/${repo}/pulls/${numbers[repo]}/files`, |
| 29 | 31 | { headers: { cookie: `forge_session=${createSession(app.db, alice.id).token}` } }, |
| 30 | 32 | ); |
| 31 | const body = await res.text(); | |
| 33 | return res.text(); | |
outdated
A selector rename only; nothing here would fail without the change. Add a test asserting |
||
| 34 | }; | |
| 35 | ||
| 36 | /** The class list of the card whose head names `heading`. */ | |
| 37 | const cardClassOf = (body: string, heading: string): string => { | |
| 38 | const at = body.indexOf(`<h2>${heading}`); | |
| 39 | expect(at).toBeGreaterThan(-1); | |
| 40 | const before = [...body.slice(0, at).matchAll(/class="(card(?: [^"]*)?)"/g)]; | |
| 41 | return before.at(-1)?.[1] ?? ""; | |
| 42 | }; | |
| 43 | ||
| 44 | const boxOf = async (repo: string): Promise<string> => { | |
| 45 | const body = await pageOf(repo); | |
| 32 | 46 | expect(body).not.toMatch(/\bmerge-(?:box|form|headline|sub)\b/); |
| 33 | const at = body.search(/<div\s+class="card mt"[^>]*data-tone=/); | |
| 47 | const at = body.search(/<div\s+class="card mt bare"\s*>\s*<ul class="list setting-list">/); | |
| 34 | 48 | expect(at).toBeGreaterThan(-1); |
| 35 | 49 | return body.slice(at, body.indexOf("</ul>", at)).replace(/\s+/g, " "); |
| 36 | 50 | }; |
| @@ -50,7 +64,7 @@ beforeAll(async () => { | ||
| 50 | 64 | }); |
| 51 | 65 | if (!made.ok) throw new Error(String(made.error)); |
| 52 | 66 | alice = made.value; |
| 53 | for (const name of ["ready", "checks", "merged"]) { | |
| 67 | for (const name of ["ready", "checks", "merged", "closed", "sliced", "fork"]) { | |
| 54 | 68 | const repo = await createRepo(app, alice, { name, visibility: "public" }); |
| 55 | 69 | if (!repo.ok) throw new Error(String(repo.error)); |
| 56 | 70 | for (const [branch, file] of [[undefined, "forge.json"], ["topic", "t.txt"]] as const) { |
| @@ -61,6 +75,14 @@ beforeAll(async () => { | ||
| 61 | 75 | }); |
| 62 | 76 | if (!done.ok) throw new Error(JSON.stringify(done.error)); |
| 63 | 77 | } |
| 78 | if (name === "sliced") { | |
| 79 | const more = await commitToBranch(app, alice, repo.value, { | |
| 80 | branch: "topic", | |
| 81 | message: "a second change", | |
| 82 | files: [{ path: "u.txt", content: "y" }], | |
| 83 | }); | |
| 84 | if (!more.ok) throw new Error(JSON.stringify(more.error)); | |
| 85 | } | |
| 64 | 86 | const pull = await openPull(app, alice, repo.value, { |
| 65 | 87 | title: name, |
| 66 | 88 | body: "", |
| @@ -71,6 +93,10 @@ beforeAll(async () => { | ||
| 71 | 93 | numbers[name] = pull.value.number; |
| 72 | 94 | if (name === "checks") |
| 73 | 95 | app.db.run("UPDATE repo SET require_checks = 1 WHERE id = ?", [repo.value.id]); |
| 96 | if (name === "closed") | |
| 97 | app.db.run("UPDATE issue SET state = 'closed' WHERE id = ?", [pull.value.id]); | |
| 98 | if (name === "fork") | |
| 99 | app.db.run("UPDATE pull SET origin_head_repo = 'bob/fork' WHERE issue_id = ?", [pull.value.id]); | |
| 74 | 100 | if (name === "merged") { |
| 75 | 101 | const done = await mergePull(app, alice, repo.value, pull.value, "merge"); |
| 76 | 102 | if (!done.ok) throw new Error(JSON.stringify(done.error)); |
| @@ -114,3 +140,34 @@ test("merged: the merged tone and the sentence", async () => { | ||
| 114 | 140 | expect(box).toContain("Merged."); |
| 115 | 141 | expect(box).toContain("was merged into"); |
| 116 | 142 | }); |
| 143 | ||
| 144 | ||
| 145 | test("the merge state is the sentence's opening word with its dot, the card bare (principle 6)", async () => { | |
| 146 | expect(await boxOf("ready")).toMatch(/class="dot" data-tone="success" ><\/span> Ready to merge/); | |
| 147 | expect(await boxOf("checks")).toMatch(/data-tone="attn" ><\/span> Waiting on checks\./); | |
| 148 | expect(await boxOf("merged")).toMatch(/data-tone="merged" ><\/span> Merged\./); | |
| 149 | // Closed has no tone: a plain dot, and the word still says it. | |
| 150 | expect(await boxOf("closed")).toMatch(/class="dot" ><\/span> Closed without merging\./); | |
| 151 | expect(await boxOf("fork")).toMatch(/data-tone="attn" ><\/span> Merge on the origin\./); | |
| 152 | }); | |
| 153 | ||
| 154 | test("the review, commits, slices and run-this-code cards are bare", async () => { | |
| 155 | const sliced = await pageOf("sliced"); | |
| 156 | expect(cardClassOf(sliced, "Review")).toMatch(/\bbare\b/); | |
| 157 | expect(cardClassOf(sliced, "2 commits")).toMatch(/\bbare\b/); | |
| 158 | expect(cardClassOf(sliced, "Slices")).toMatch(/\bbare\b/); | |
| 159 | expect(cardClassOf(await pageOf("fork"), "Run this code?")).toMatch(/\bbare\b/); | |
| 160 | }); | |
| 161 | ||
| 162 | test("the team verdicts are a plain list of rows, not a card (principle 7)", () => { | |
| 163 | const verdicts: TeamVerdict[] = [ | |
| 164 | { team: "core", name: "Core", state: "approved", by: "bob", stale: false }, | |
| 165 | { team: "docs", name: "Docs", state: undefined, by: undefined, stale: false }, | |
| 166 | ]; | |
| 167 | const out = String(teamsCard(verdicts)); | |
| 168 | expect(out).toContain('class="team-verdicts mt"'); | |
| 169 | expect(out).toContain('<ul class="list">'); | |
| 170 | expect(out).not.toMatch(/class="[^"]*\bcard\b/); | |
| 171 | expect(out).toContain("approved by bob"); | |
| 172 | expect(out).toContain("awaiting a reviewer"); | |
| 173 | }); |
Principle 6: the tone is passed but a bare card draws no border, so merge state loses its only signal. Show it as a word/dot in the first row, and test it.