Public: anyone can read this; the crew can change it.
Editing and deleting a file in the browser speak the home page's language #9911
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9849-editing-and-deleting-a-file-in-the-brows → main 3 commits fork 2 threads · 2 unresolved
3 files
changed +67
−52
docs/design/areas/edit.md +4
| @@ -6,3 +6,7 @@ branch input live outside `field()` (the path sits in the editor's head row, wit | ||
| 6 | 6 | row is written by hand to the same shape. An error that belongs to no input (not allowed, branch moved, origin |
| 7 | 7 | unreachable) is the page's one flash, `tone=danger`, passed to `page()` as `flash` because the form re-renders in place |
| 8 | 8 | instead of redirecting. |
| 9 | ||
| 10 | The commit section of the editor and the delete page add no component class. As on the home page they are a heading, plain | |
| 11 | sentences, the fields and the one button, no box: each is a `<section class="settings-block">` with an `<h2>` (the commit | |
| 12 | section keeps `.editor-commit` for its spacing). Field names and the `data-preview`/`data-path` hooks are unchanged. |
src/web/pages/edit.ts +47 −52
| @@ -41,7 +41,7 @@ import { | ||
| 41 | 41 | type ProposeTarget, |
| 42 | 42 | type RepoScope, |
| 43 | 43 | } from "../scope.ts"; |
| 44 | import { btn, card, field, icon, sizeWithTokens } from "../ui.ts"; | |
| 44 | import { btn, field, icon, sizeWithTokens } from "../ui.ts"; | |
| 45 | 45 | import { pathCrumbs, refSetFrom } from "./refs.ts"; |
| 46 | 46 | |
| 47 | 47 | /** What the form is holding, so a refusal can hand every word back. */ |
| @@ -294,8 +294,9 @@ ${draft.content}</textarea | ||
| 294 | 294 | hidden |
| 295 | 295 | ></div> |
| 296 | 296 | |
| 297 | ${card( | |
| 298 | html`${field({ | |
| 297 | <section class="settings-block editor-commit"> | |
| 298 | <h2>${draft.creating ? "Create the file" : "Commit the change"}</h2> | |
| 299 | ${field({ | |
| 299 | 300 | label: "Commit message", |
| 300 | 301 | name: "message", |
| 301 | 302 | value: draft.subject, |
| @@ -341,9 +342,8 @@ ${draft.content}</textarea | ||
| 341 | 342 | icon: "trash", |
| 342 | 343 | }), |
| 343 | 344 | )} |
| 344 | </div>`, | |
| 345 | { extra: "editor-commit" }, | |
| 346 | )} | |
| 345 | </div> | |
| 346 | </section> | |
| 347 | 347 | </form>`; |
| 348 | 348 | } |
| 349 | 349 | |
| @@ -955,53 +955,48 @@ export async function deleteFilePage(ctx: Ctx): Promise<Response> { | ||
| 955 | 955 | palette: scope.palette, |
| 956 | 956 | body: html`<div class="narrow-wide"> |
| 957 | 957 | ${pageHead(html`Delete <span class="mono">${path}</span>?`)} |
| 958 | ${card( | |
| 959 | html`<p> | |
| 960 | It is removed from | |
| 961 | <span class="mono">${mustBranch ? "a new branch" : ref}</span> in | |
| 962 | one commit. Every other branch keeps it, and so does every commit | |
| 963 | that already exists — this is a deletion in history, not from it. | |
| 964 | </p> | |
| 965 | <form method="post" action="${scope.base}/delete" class="mt"> | |
| 966 | <input type="hidden" name="ref" value="${ref}" /> | |
| 967 | <input type="hidden" name="path" value="${path}" /> | |
| 968 | <input | |
| 969 | type="hidden" | |
| 970 | name="target" | |
| 971 | value="${mustBranch ? "new" : "current"}" | |
| 972 | /> | |
| 973 | ${when( | |
| 974 | mustBranch, | |
| 975 | () => | |
| 976 | html`<input | |
| 977 | type="hidden" | |
| 978 | name="branch" | |
| 979 | value="${suggestBranch( | |
| 980 | ctx.auth.user?.username ?? "edit", | |
| 981 | path, | |
| 982 | )}" | |
| 983 | />`, | |
| 984 | )} | |
| 985 | ${field({ | |
| 986 | label: "Commit message", | |
| 987 | name: "message", | |
| 988 | value: `Delete ${path}`, | |
| 989 | required: true, | |
| 958 | <section class="settings-block" id="delete-file"> | |
| 959 | <p> | |
outdated
Principle 1: this h2 repeats the page head ("Delete <path>?"). Remove it and let the first sentence open the section; nothing should explain itself twice. |
||
| 960 | It is removed from | |
| 961 | <span class="mono">${mustBranch ? "a new branch" : ref}</span> in one | |
| 962 | commit. Every other branch keeps it, and so does every commit that | |
| 963 | already exists — this is a deletion in history, not from it. | |
| 964 | </p> | |
| 965 | <form method="post" action="${scope.base}/delete" class="mt"> | |
| 966 | <input type="hidden" name="ref" value="${ref}" /> | |
| 967 | <input type="hidden" name="path" value="${path}" /> | |
| 968 | <input | |
| 969 | type="hidden" | |
| 970 | name="target" | |
| 971 | value="${mustBranch ? "new" : "current"}" | |
| 972 | /> | |
| 973 | ${when( | |
| 974 | mustBranch, | |
| 975 | () => | |
| 976 | html`<input | |
| 977 | type="hidden" | |
| 978 | name="branch" | |
| 979 | value="${suggestBranch( | |
| 980 | ctx.auth.user?.username ?? "edit", | |
| 981 | path, | |
| 982 | )}" | |
| 983 | />`, | |
| 984 | )} | |
| 985 | ${field({ | |
| 986 | label: "Commit message", | |
| 987 | name: "message", | |
| 988 | value: `Delete ${path}`, | |
| 989 | required: true, | |
| 990 | })} | |
| 991 | <div class="form-actions"> | |
| 992 | ${btn("Delete this file", { variant: "danger" })} | |
| 993 | ${btn("Cancel", { | |
| 994 | href: `${scope.base}/blob/${encoded}/${path}`, | |
| 995 | variant: "ghost", | |
| 990 | 996 | })} |
| 991 | <div class="form-actions"> | |
| 992 | ${btn("Delete this file", { variant: "danger" })} | |
| 993 | ${btn("Cancel", { | |
| 994 | href: `${scope.base}/blob/${encoded}/${path}`, | |
| 995 | variant: "ghost", | |
| 996 | })} | |
| 997 | </div> | |
| 998 | </form>`, | |
| 999 | { | |
| 1000 | tone: "danger", | |
| 1001 | head: html`${icon("alert")} | |
| 1002 | <h2>Delete a file</h2>`, | |
| 1003 | }, | |
| 1004 | )} | |
| 997 | </div> | |
| 998 | </form> | |
| 999 | </section> | |
| 1005 | 1000 | </div>`, |
| 1006 | 1001 | }); |
| 1007 | 1002 | } |
test/web-editor.test.ts +16
| @@ -536,6 +536,20 @@ describe("editing files on the web", () => { | ||
| 536 | 536 | expect(await res.text()).toContain("path this repository can hold"); |
| 537 | 537 | }); |
| 538 | 538 | |
| 539 | test("the commit section and the delete page are not boxed in cards", async () => { | |
| 540 | const edit = await text("/alice/beacon/edit/main/src/lamp.ts", alice); | |
| 541 | const del = await text("/alice/beacon/delete/main/src/spare.ts", alice); | |
| 542 | for (const html of [edit, del]) { | |
| 543 | expect(html).not.toContain('class="card'); | |
| 544 | expect(html).toContain('class="settings-block'); | |
| 545 | } | |
| 546 | expect(edit).toContain('action="/alice/beacon/edit"'); | |
| 547 | expect(edit).toContain('name="message"'); | |
| 548 | expect(edit).toContain("data-preview="); | |
| 549 | expect(del).toContain('action="/alice/beacon/delete"'); | |
| 550 | expect(del).toContain('name="message"'); | |
| 551 | }); | |
| 552 | ||
| 539 | 553 | // ---- deleting ---- |
| 540 | 554 | |
| 541 | 555 | test("deleting a file goes through its own confirm page", async () => { |
| @@ -547,6 +561,8 @@ describe("editing files on the web", () => { | ||
| 547 | 561 | expect(html).toContain('action="/alice/beacon/delete"'); |
| 548 | 562 | // The confirm page is where the deletion is described, not the button. |
| 549 | 563 | expect(html).toContain("a deletion in history, not from it"); |
| 564 | // The page head says it is a deletion; no second heading repeats it. | |
| 565 | expect(html).not.toContain("Delete a file"); | |
| 550 | 566 | |
| 551 | 567 | const res = await post( |
| 552 | 568 | "/alice/beacon/delete", |
Non-blocking: this says both sections carry an <h2>, but the delete page now opens on its paragraph with no heading. Say so (principle 1).