Public: anyone can read this; the crew can change it.
The commits list and a commit's page speak the home page's language #9778
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9742-the-commits-list-and-a-commit-s-page-spe → main 4 commits fork 4 threads · 4 unresolved
3 files
changed +79
−89
src/web/pages/commits.ts +49 −80
| @@ -30,10 +30,7 @@ import { localScope, type RepoScope } from "../scope.ts"; | ||
| 30 | 30 | import { |
| 31 | 31 | avatar, |
| 32 | 32 | badge, |
| 33 | card, | |
| 34 | 33 | empty, |
| 35 | icon, | |
| 36 | listCard, | |
| 37 | 34 | messageText, |
| 38 | 35 | originTag, |
| 39 | 36 | pager, |
| @@ -59,8 +56,7 @@ const PER_PAGE = 30; | ||
| 59 | 56 | */ |
| 60 | 57 | export const COMMITS_CSS = `[data-commits] { overflow-wrap: anywhere; } |
| 61 | 58 | [data-commits] .row-title, [data-commits] .row-meta { white-space: normal; overflow: visible; text-overflow: clip; } |
| 62 | [data-commits] .row { align-items: flex-start; } | |
| 63 | [data-commits] .card-head h2 { min-width: 0; }`; | |
| 59 | [data-commits] .row { align-items: flex-start; }`; | |
| 64 | 60 | |
| 65 | 61 | /** Infinity, 1e300 and NaN all collapse to page 1 — never a bogus skip. */ |
| 66 | 62 | function pageNumber(raw: string | null): number { |
outdated
Principles 4 and 8 / tokens: a literal 1.25rem font-size in page CSS. Use the type token the home page uses for a heading instead of a private size, or no rule at all. |
||
| @@ -82,58 +78,38 @@ export function commitRows( | ||
| 82 | 78 | commits: readonly Commit[], |
| 83 | 79 | verdicts: ReadonlyMap<string, CommitCheck> = new Map(), |
| 84 | 80 | ): Html { |
| 85 | const glyph = scope.origin?.kind ?? scope.repo?.origin?.kind ?? "globe"; | |
| 86 | const lead = (commit: Commit): Html => | |
| 87 | html`<span class="row-lead" | |
| 88 | >${scope.kind === "origin" && commit.login | |
| 89 | ? icon(glyph) | |
| 90 | : avatar(commit.author)}</span | |
| 91 | >`; | |
| 92 | const text = (commit: Commit): Html => | |
| 93 | html`<span class="row-title" title="${commit.subject}">${commit.subject}</span> | |
| 94 | <span class="row-meta" | |
| 95 | >${commit.author} committed | |
| 96 | ${time(commit.date)}${when( | |
| 97 | commit.parents.length > 1, | |
| 98 | () => html` · ${badge("merge", { variant: "secondary" })}`, | |
| 99 | )}</span | |
| 100 | >`; | |
| 101 | return html`<div data-commits>${listCard( | |
| 81 | return html`<div data-commits><ul class="list">${ | |
| 102 | 82 | commits.map((commit) => { |
| 103 | 83 | const verdict = verdicts.get(commit.oid); |
| 104 | const step = verdict?.summary.split("\n")[0]?.trim() ?? ""; | |
| 105 | // A link cannot sit inside a link: a row with a verdict is a row whose | |
| 106 | // title links to the commit and whose badge links to the check. | |
| 107 | if (verdict === undefined || verdict.runId === null) | |
| 108 | return html`<li> | |
| 109 | <a | |
| 110 | class="row" | |
| 111 | data-nav | |
| 112 | tabindex="-1" | |
| 113 | href="${scope.base}/commit/${commit.oid}" | |
| 114 | > | |
| 115 | ${lead(commit)}<span class="row-main">${text(commit)}</span> | |
| 116 | <span class="row-trail">${sha(commit.oid)}</span> | |
| 117 | </a> | |
| 118 | </li>`; | |
| 84 | // The message is the row's one link; the check is a plain badge here and | |
| 85 | // its run is reachable from the commit page. | |
| 119 | 86 | return html`<li> |
| 120 | <div class="row" data-nav tabindex="-1"> | |
| 121 | ${lead(commit)} | |
| 122 | <a class="row-main" href="${scope.base}/commit/${commit.oid}" | |
| 123 | >${text(commit)}</a | |
| 87 | <a | |
| 88 | class="row" | |
| 89 | data-nav | |
| 90 | tabindex="-1" | |
| 91 | href="${scope.base}/commit/${commit.oid}" | |
| 92 | ><span class="row-main" | |
| 93 | ><span class="row-title" title="${commit.subject}">${commit.subject}</span> | |
| 94 | <span class="row-meta" | |
| 95 | >${commit.author} committed | |
| 96 | ${time(commit.date)}${when( | |
| 97 | commit.parents.length > 1, | |
| 98 | () => html` · ${badge("merge", { variant: "secondary" })}`, | |
| 99 | )}</span | |
| 100 | ></span | |
outdated
Principle 7 and box 1: the rows keep lead(commit) (avatar/origin icon) and a second link (the check badge) beside the message link, so the message is not the one link and the row is not the home page row. Make the message the only link; show the check as plain text/badge (non-link) with the run reachable from the commit page. Also drop the avatar from the row; author goes in the muted meta line. Add a test that the list has no card markup and that each row has exactly one anchor. |
||
| 124 | 101 | > |
| 125 | 102 | <span class="row-trail" |
| 126 | ><a | |
| 127 | href="${scope.base}/runs/${verdict.runId}" | |
| 128 | title="${step}" | |
| 129 | aria-label="Check ${verdict.state === "ok" ? "passed" : "failed"} for ${commit.oid.slice(0, 7)}${step === "" ? "" : `: ${step}`}" | |
| 130 | >${runStateBadge(verdict.state === "ok" ? "ok" : "failed", "check")}</a | |
| 131 | >${sha(commit.oid)}</span | |
| 103 | >${when( | |
| 104 | verdict !== undefined && verdict.runId !== null, | |
| 105 | () => | |
| 106 | html`${runStateBadge(verdict!.state === "ok" ? "ok" : "failed", "check")} `, | |
| 107 | )}${sha(commit.oid)}</span | |
| 132 | 108 | > |
| 133 | </div> | |
| 109 | </a> | |
| 134 | 110 | </li>`; |
| 135 | }), | |
| 136 | )}</div>`; | |
| 111 | }) | |
| 112 | }</ul></div>`; | |
| 137 | 113 | } |
| 138 | 114 | |
| 139 | 115 | /** A commit's check in a sentence; on red, the first failing step as the check named it. */ |
| @@ -174,38 +150,31 @@ export function renderCommit( | ||
| 174 | 150 | ): Html { |
| 175 | 151 | const verdict: Html = |
| 176 | 152 | check !== null && check.state !== "none" |
| 177 | ? html`<div class="card-body small">${checkSentence(scope, check)}</div>` | |
| 153 | ? html`<p class="small">${checkSentence(scope, check)}</p>` | |
| 178 | 154 | : html``; |
| 179 | const head: Html = card( | |
| 180 | html`${verdict}${when( | |
| 181 | commit.body.trim() !== "", | |
| 182 | () => html`<div class="card-body">${messageText(commit.body)}</div>`, | |
| 155 | const head: Html = html`<h2>${commit.subject}</h2> | |
| 156 | ${verdict} | |
| 157 | ${when( | |
| 158 | commit.body.trim() !== "", | |
| 159 | () => messageText(commit.body), | |
| 160 | )} | |
| 161 | <p class="bar small muted"> | |
| 162 | ${commit.login | |
| 163 | ? originTag( | |
| 164 | commit.login, | |
| 165 | scope.origin?.kind ?? scope.repo?.origin?.kind ?? "globe", | |
| 166 | ) | |
| 167 | : avatar(commit.author)} | |
| 168 | <span>${commit.author} committed ${time(commit.date)}</span> | |
| 169 | ${sha(commit.oid)} | |
| 170 | ${when( | |
| 171 | commit.parents.length > 1, | |
| 172 | () => html`${badge("merge", { icon: "merge" })}`, | |
| 183 | 173 | )} |
| 184 | <div class="card-body bar small muted"> | |
| 185 | ${commit.login | |
| 186 | ? originTag( | |
| 187 | commit.login, | |
| 188 | scope.origin?.kind ?? scope.repo?.origin?.kind ?? "globe", | |
| 189 | ) | |
| 190 | : avatar(commit.author)} | |
| 191 | <span>${commit.author} committed ${time(commit.date)}</span> | |
| 192 | ${sha(commit.oid)} | |
| 193 | ${when( | |
| 194 | commit.parents.length > 1, | |
| 195 | () => html`${badge("merge", { icon: "merge" })}`, | |
| 196 | )} | |
| 197 | <span class="spacer"></span> | |
| 198 | <a | |
| 199 | class="btn" | |
| 200 | data-size="sm" | |
| 201 | data-variant="outline" | |
| 202 | href="${scope.base}/tree/${commit.oid}" | |
| 203 | >${icon("repo")}Browse files</a | |
| 204 | > | |
| 205 | </div>`, | |
| 206 | { head: html`<h2>${commit.subject}</h2>`, pad: false }, | |
| 207 | ); | |
| 208 | return html`<div data-commits><div class="mt">${head}</div></div> | |
| 174 | <span class="spacer"></span> | |
| 175 | <a href="${scope.base}/tree/${commit.oid}">Browse files</a> | |
| 176 | </p>`; | |
| 177 | return html`<div data-commits>${head}</div> | |
| 209 | 178 | <div class="mt"> |
outdated
Principle 1: the check sentence was the first thing on the commit page and is now last, below the body and meta, so a red commit is easy to miss (box 2: nothing lost). Put the check line directly under the subject. Also add a test asserting the commit page has no card and keeps its check sentence for green, red and not-run. |
||
| 210 | 179 | ${renderDiff(diff, { |
| 211 | 180 | syntax, |
test/commit-page-timing.test.ts +3 −2
| @@ -104,11 +104,12 @@ test("a commit's head draws §5's bar, not .issue-meta", async () => { | ||
| 104 | 104 | expect(body).toContain(oid.slice(0, 7)); |
| 105 | 105 | }); |
| 106 | 106 | |
| 107 | test("a commit's head and body draw §5's card-head and prose, not commit-head/commit-body", async () => { | |
| 107 | test("a commit's head and body draw a heading and prose, not commit-head/commit-body", async () => { | |
| 108 | 108 | const body = await (await fetch(`${servers.url}/ada/engine/commit/${oid}`)).text(); |
| 109 | 109 | expect(body).not.toContain("commit-head"); |
| 110 | 110 | expect(body).not.toContain("commit-body"); |
| 111 | expect(body).toContain('<div class="card-head"><h2>add a</h2></div>'); | |
| 111 | expect(body).toContain("<h2>add a</h2>"); | |
| 112 | expect(body).not.toContain("card-head"); | |
| 112 | 113 | }); |
| 113 | 114 | |
| 114 | 115 | test("a commit with a body shows it, escaped, inside the prose message div", async () => { |
test/web-commits-check.test.ts +27 −7
| @@ -84,15 +84,15 @@ const get = async (path: string, cookie?: string) => | ||
| 84 | 84 | const rowOf = (page: string, subject: string): string => |
| 85 | 85 | (page.split("<li>").find((l) => l.includes(subject)) ?? "").split("</li>")[0]!; |
| 86 | 86 | |
| 87 | test("the list shows green and red linked to their runs, and nothing without a check", async () => { | |
| 87 | test("the list shows green and red as plain badges, and nothing without a check", async () => { | |
| 88 | 88 | const page = await get("/alice/pub/commits"); |
| 89 | 89 | const green = rowOf(page, "green change"); |
| 90 | 90 | const red = rowOf(page, "red change"); |
| 91 | 91 | const none = rowOf(page, "none change"); |
| 92 | 92 | expect(green).toContain("Passed"); |
| 93 | expect(green).toContain(`href="/alice/pub/runs/${runs.green}"`); | |
| 94 | 93 | expect(red).toContain("Failed"); |
| 95 | expect(red).toContain(`href="/alice/pub/runs/${runs.red}"`); | |
| 94 | expect(green).not.toContain("/runs/"); | |
| 95 | expect(red).not.toContain("/runs/"); | |
| 96 | 96 | expect(none).not.toContain("/runs/"); |
| 97 | 97 | expect(none).not.toContain("badge"); |
| 98 | 98 | }); |
| @@ -125,7 +125,6 @@ test("a private repository's verdict is the owner's alone", async () => { | ||
| 125 | 125 | expect(seen).not.toContain("step secret failed"); |
| 126 | 126 | } |
| 127 | 127 | const mine = await get(list, ownerCookie); |
| 128 | expect(mine).toContain(`href="/alice/priv/runs/${runs.priv}"`); | |
| 129 | 128 | expect(mine).toContain("Failed"); |
| 130 | 129 | expect(await get(one, ownerCookie)).toContain("first failing step secret failed"); |
| 131 | 130 | }); |
| @@ -153,7 +152,28 @@ test("the sentence names whose check answered, a running check and a superseded | ||
| 153 | 152 | expect(gone).not.toContain("/runs/"); |
| 154 | 153 | }); |
| 155 | 154 | |
| 156 | test("the badge link names the commit and the failing step", async () => { | |
| 157 | const red = rowOf(await get("/alice/pub/commits"), "red change"); | |
| 158 | expect(red).toContain(`aria-label="Check failed for ${oids["pub/red"]!.slice(0, 7)}: step typecheck failed"`); | |
| 155 | test("the list has no card markup and each row has exactly one anchor and no avatar", async () => { | |
| 156 | const page = await get("/alice/pub/commits"); | |
| 157 | expect(page).not.toContain('class="card'); | |
| 158 | const rows = page.split("<li>").slice(1).map((l) => l.split("</li>")[0]!).filter((l) => l.includes('class="row"')); | |
| 159 | expect(rows.length).toBe(3); | |
| 160 | for (const row of rows) { | |
| 161 | expect(row.match(/<a[\s>]/g)?.length).toBe(1); | |
| 162 | expect(row).not.toContain("avatar"); | |
| 163 | expect(row).not.toContain("row-lead"); | |
| 164 | } | |
| 165 | }); | |
| 166 | ||
| 167 | test("a commit's page has no card and its check line follows the subject for green, red and not-run", async () => { | |
| 168 | for (const [n, said] of [["green", "The check passed"], ["red", "The check failed"], ["none", ""]] as const) { | |
| 169 | const one = await get(`/alice/pub/commit/${oids[`pub/${n}`]}`); | |
| 170 | expect(one).not.toContain('class="card'); | |
| 171 | const afterSubject = one.slice(one.indexOf(`${n} change</h2>`)); | |
| 172 | if (said === "") expect(one).not.toContain("The check"); | |
| 173 | else { | |
| 174 | const at = afterSubject.indexOf(said); | |
| 175 | expect(at).toBeGreaterThan(0); | |
| 176 | expect(at).toBeLessThan(afterSubject.indexOf("committed")); | |
| 177 | } | |
| 178 | } | |
| 159 | 179 | }); |
Principles 3 and 6: a hand-rolled border-top on .list in page CSS. If the list needs a hairline, it belongs to the named list component, not an override in this page. Drop it and let space separate the list from the heading.