Public: anyone can read this; the crew can change it.
A review comment on a diff line reads as plain prose under its author, not a boxed card #10062
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/10045-a-review-comment-on-a-diff-line-reads-as → main 3 commits fork 3 threads · 3 unresolved
4 files
changed +16
−9
docs/design/areas/diff.md +1
| @@ -13,6 +13,7 @@ file. | ||
| 13 | 13 | - `.diff-note` — a centred muted line in place of rows: a binary file or no content changes. Files past the cap are not a `.diff-note`: the cut is a plain `p.muted.small` line under the last file, with its own 12px top margin. |
| 14 | 14 | - `.diff-stub` — a `.diff-note` for a file too large or generated to draw: why, its real counts and a View file button. |
| 15 | 15 | - `.dn-comment` — a line number in the gutter that links to commenting on that line, when the diff is reviewable. |
| 16 | - `.dc-thread` — an open line-comment thread under its row: each comment is a muted author·time·"outdated" line (`.comment-head`) over its prose, with no card; the Resolve button follows. A resolved thread stays a card with its disclosure. | |
| 16 | 17 | - `.nonl` — the `↵` mark after a line that has no newline at the end of the file. |
| 17 | 18 | - `.diff` — the wrapper around one whole diff: its summary and every file. |
| 18 | 19 | - `.dl` — one line of a hunk, a table row; its `data-k` says whether it is context, added or deleted. |
src/web/diff-view.ts +4 −4
| @@ -313,8 +313,8 @@ function reviewRows( | ||
| 313 | 313 | const currentHash = hashLine(currentText); |
| 314 | 314 | const thread = comments.map((c) => { |
| 315 | 315 | const outdated = c.lineHash !== currentHash; |
| 316 | return html`<div class="card comment"> | |
| 317 | <div class="card-head"> | |
| 316 | return html`<div class="comment-row"> | |
| 317 | <div class="comment-head muted small"> | |
| 318 | 318 | ${userLink(c.authorName, { mark: opts.marks?.get(c.authorName) })} |
| 319 | 319 | ${time(c.createdAt)} |
| 320 | 320 | ${when( |
| @@ -328,7 +328,7 @@ function reviewRows( | ||
| 328 | 328 | |
| 329 | 329 | const form = isActive |
| 330 | 330 | ? html`<form |
| 331 | class="card comment-form" | |
| 331 | class="comment-form" | |
| 332 | 332 | data-submit-key |
| 333 | 333 | method="post" |
| 334 | 334 | action="${opts.reviewBase}/review" |
| @@ -395,7 +395,7 @@ function reviewRows( | ||
| 395 | 395 | )}`, |
| 396 | 396 | toggle ? { foot: toggle } : {}, |
| 397 | 397 | ) |
| 398 | : card(html`${thread}`, toggle ? { foot: toggle } : {}); | |
| 398 | : html`<div class="dc-thread">${thread}${toggle}</div>`; | |
| 399 | 399 | |
| 400 | 400 | return html`<tr> |
| 401 | 401 | <td class="dn"></td> |
src/web/style/content.ts +5 −2
| @@ -621,8 +621,11 @@ form.inline { display: inline; margin-left: auto; } | ||
| 621 | 621 | padding: 8px 12px; background: var(--muted); |
| 622 | 622 | border-left: 3px solid var(--accent); white-space: normal; font-family: var(--font); |
| 623 | 623 | } |
Principle 3/6: |
||
| 624 | .dc-review .card + .card, .dc-review .comment { margin-bottom: 8px; } | |
| 625 | .dc-review .comment-form { margin-top: 0; } | |
| 624 | .dc-review .comment-form { margin: 0; padding: 0; } | |
| 625 | /* An open thread is prose under its line, not a card: head and body start at the code column, | |
| 626 | since .dc-review pads 12px past the 3px edge exactly as .dc does. */ | |
outdated
The issue asks for the body indented to the diff gutter; |
||
| 627 | .dc-thread { display: grid; gap: 10px; min-width: 0; } | |
| 628 | .dc-thread .comment-body { overflow-wrap: anywhere; } | |
| 626 | 629 | /* A settled thread is one line plus a disclosure: the point of resolving is |
| 627 | 630 | to get an answered question out of the way of the unanswered ones. */ |
| 628 | 631 | .dc-review .card-body > .muted .ico { width: 14px; height: 14px; color: var(--success); } |
test/diff-review-comment.test.ts +6 −3
| @@ -56,7 +56,9 @@ describe("inline review comments use the comment component", () => { | ||
| 56 | 56 | test("the text, the outdated mark, the resolved line and the reply form are", () => { |
| 57 | 57 | expect(out).toContain("<strong>question</strong>"); |
| 58 | 58 | expect(out).toContain("stale remark"); |
| 59 | expect(out).toContain('class="card comment'); | |
| 59 | expect(out).toContain('class="comment-row"'); | |
| 60 | expect(out).not.toContain('class="card comment'); | |
| 61 | expect(out).not.toContain('class="card comment-form'); | |
| 60 | 62 | expect(out).toContain("outdated"); |
| 61 | 63 | expect(out).toContain("resolved"); |
| 62 | 64 | expect(out).toContain('action="/o/r/pulls/1/review"'); |
| @@ -76,11 +78,12 @@ describe("a review thread is a card and its resolve control a button", () => { | ||
| 76 | 78 | ]), |
| 77 | 79 | }).toString(); |
| 78 | 80 | |
| 79 | test("an open thread is a card with a Resolve button", () => { | |
| 81 | test("an open thread is prose with a Resolve button", () => { | |
| 80 | 82 | const out = render(null); |
| 81 | 83 | expect(out).toContain('action="/o/r/pulls/1/threads/x/resolve"'); |
| 82 | 84 | expect(out).toMatch(/<button[^>]*class="btn"[^>]*>\s*Resolve\s*<\/button>/); |
| 83 | expect(out).toContain('class="card-foot"'); | |
| 85 | expect(out).toContain('class="dc-thread"'); | |
| 86 | expect(out).not.toContain("card-head"); | |
| 84 | 87 | }); |
| 85 | 88 | |
| 86 | 89 | test("a resolved thread is one line with an Unresolve button", () => { |
Good: comment-row/comment-head reuse the existing component. Note the resolved-thread card still wraps these rows, so the Show-comments body is prose inside a card; fine, but the look must show both states.