Public: anyone can read this; the crew can change it.
The new issue and new pull forms speak the home page's language #9724
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9719-the-new-issue-and-new-pull-forms-speak-t → main 4 commits fork 3 threads · 3 unresolved
4 files
changed +151
−49
docs/design/areas/forms.md +1 −1
| @@ -2,7 +2,7 @@ | ||
| 2 | 2 | |
| 3 | 3 | A form's fields and its buttons sit in two rows, one for what you fill in and one for what you do with it. |
| 4 | 4 | |
| 5 | - `.field-row` — a row of `.field`s side by side that wraps on a narrow screen, each at least 160px wide and aligned to the bottom so a labelled input and the button beside it share a baseline. Styled in `style/components.ts`; written by `createForm` in `pages/branches.ts` (the new-branch form), the graph filter in `pages/fleet-graph.ts`, the freeze form in `pages/frozen.ts` and the new-pull form in `pages/pulls.ts`. | |
| 5 | - `.field-row` — a row of `.field`s side by side that wraps on a narrow screen, each at least 160px wide and aligned to the bottom so a labelled input and the button beside it share a baseline. Styled in `style/components.ts`; written by `createForm` in `pages/branches.ts` (the new-branch form), the graph filter in `pages/fleet-graph.ts` and the freeze form in `pages/frozen.ts`. The new-pull form stacks Base and Compare as plain `.field`s, one column at any width. | |
| 6 | 6 | - `.form-actions` — the row of a form's buttons under its fields: 16px above, 8px apart, wrapping. Styled in `style/components.ts`, and in `style/content.ts` for a thread's close reason and "close into" input and for `.comment-form` on a narrow screen; written by nearly every form in `src/web/pages/` (`account.ts`, `settings.ts`, `crew.ts`, `edit.ts`, `issues.ts`, `pulls.ts`, `oauth.ts`, `teams.ts`, `chain.ts`, `everyday.ts`) and by the review comment form in `diff-view.ts`; `client.ts` finds its `.muted` note to report a save. |
| 7 | 7 | |
| 8 | 8 | - The gate break-glass (#7790) on the pull's Files step is a plain `field()` with a hint and a `.form-actions` row holding one `btn` in the `danger` variant; no new class. It sits under the disabled primary Merge button, so the page keeps one accent. |
src/web/pages/issues.ts +1 −1
| @@ -127,7 +127,7 @@ function newIssueForm( | ||
| 127 | 127 | </div> |
| 128 | 128 | <div class="form-actions"> |
| 129 | 129 | ${btn(submit, { variant: "primary", icon: "issue" })} |
| 130 | ${btn("Cancel", { href: cancelHref, variant: "ghost" })} | |
| 130 | ${btn("Cancel", { href: cancelHref, variant: "link" })} | |
| 131 | 131 | </div> |
| 132 | 132 | </form> |
| 133 | 133 | </div>`; |
src/web/pages/pulls.ts +36 −47
| @@ -30,7 +30,7 @@ import { | ||
| 30 | 30 | import { field, readForm } from "../form.ts"; |
| 31 | 31 | import { html, raw, when, type Html } from "../html.ts"; |
| 32 | 32 | import { flashTo, page } from "../layout.ts"; |
| 33 | import { btn, card, empty, icon, field as formField } from "../ui.ts"; | |
| 33 | import { btn, empty, field as formField } from "../ui.ts"; | |
| 34 | 34 | import { refSet } from "./refs.ts"; |
| 35 | 35 | |
| 36 | 36 | /** open | closed | merged — the one place the third state is derived. */ |
| @@ -70,53 +70,42 @@ function composeForm( | ||
| 70 | 70 | const cross = parent !== undefined; |
| 71 | 71 | return html`<form method="post" action="${repoBase(repo)}/pulls/new"> |
| 72 | 72 | <input type="hidden" name="baseRepo" value="${cross ? "parent" : "self"}" /> |
| 73 | ${card( | |
| 74 | html`<div class="field-row"> | |
| 75 | <div class="field"> | |
| 76 | <label for="f-base" | |
| 77 | >Base${when( | |
| 78 | parent, | |
| 79 | (p) => | |
| 80 | html` <span class="muted mono">${p.ownerName}/${p.name}</span>`, | |
| 81 | )}</label | |
| 82 | > | |
| 83 | <select class="input" id="f-base" name="base"> | |
| 84 | ${branchOptions(baseBranches, values.base)} | |
| 85 | </select> | |
| 86 | </div> | |
| 87 | <span class="muted">${icon("merge")}</span> | |
| 88 | <div class="field"> | |
| 89 | <label for="f-head">Compare</label> | |
| 90 | <select class="input" id="f-head" name="head"> | |
| 91 | ${branchOptions(headBranches, values.head)} | |
| 92 | </select> | |
| 93 | </div> | |
| 94 | </div>`, | |
| 95 | { | |
| 96 | head: html`${icon("branch")} | |
| 97 | <h2>Branches</h2>`, | |
| 98 | }, | |
| 99 | )} | |
| 100 | <div class="mt"> | |
| 101 | ${formField({ | |
| 102 | label: "Title", | |
| 103 | name: "title", | |
| 104 | value: values.title, | |
| 105 | required: true, | |
| 106 | autofocus: true, | |
| 107 | ...(error ? { error } : {}), | |
| 108 | })} | |
| 109 | ${formField({ | |
| 110 | label: "Description", | |
| 111 | name: "body", | |
| 112 | type: "textarea", | |
| 113 | rows: 8, | |
| 114 | value: values.body, | |
| 115 | hint: "Markdown is supported.", | |
| 116 | })} | |
| 73 | <div class="field"> | |
| 74 | <label for="f-base" | |
| 75 | >Base${when( | |
| 76 | parent, | |
| 77 | (p) => | |
| 78 | html` <span class="muted mono">${p.ownerName}/${p.name}</span>`, | |
| 79 | )}</label | |
| 80 | > | |
| 81 | <select class="input" id="f-base" name="base"> | |
| 82 | ${branchOptions(baseBranches, values.base)} | |
| 83 | </select> | |
| 117 | 84 | </div> |
| 85 | <div class="field"> | |
| 86 | <label for="f-head">Compare</label> | |
| 87 | <select class="input" id="f-head" name="head"> | |
| 88 | ${branchOptions(headBranches, values.head)} | |
| 89 | </select> | |
| 90 | </div> | |
| 91 | ${formField({ | |
| 92 | label: "Title", | |
| 93 | name: "title", | |
| 94 | value: values.title, | |
| 95 | required: true, | |
| 96 | autofocus: true, | |
| 97 | ...(error ? { error } : {}), | |
| 98 | })} | |
| 99 | ${formField({ | |
| 100 | label: "Description", | |
| 101 | name: "body", | |
| 102 | type: "textarea", | |
| 103 | rows: 8, | |
| 104 | value: values.body, | |
| 105 | hint: "Markdown is supported.", | |
| 106 | })} | |
| 118 | 107 | ${confirmPublic |
| 119 | ? html`<label class="field mt"> | |
| 108 | ? html`<label class="field"> | |
| 120 | 109 | <input type="checkbox" name="confirm_public" value="1" /> |
| 121 | 110 | Publish this branch anyway: everyone who can read |
| 122 | 111 | ${repo.ownerName}/${repo.name} sees the fix, and with it where the |
| @@ -125,7 +114,7 @@ function composeForm( | ||
| 125 | 114 | : null} |
| 126 | 115 | <div class="form-actions"> |
| 127 | 116 | ${btn("Create pull request", { variant: "primary", icon: "pull" })} |
| 128 | ${btn("Cancel", { href: `${repoBase(repo)}/pulls`, variant: "ghost" })} | |
| 117 | ${btn("Cancel", { href: `${repoBase(repo)}/pulls`, variant: "link" })} | |
| 129 | 118 | </div> |
| 130 | 119 | </form>`; |
| 131 | 120 | } |
test/compose-fit.test.ts added +113
| @@ -0,0 +1,113 @@ | ||
| 1 | /** | |
| 2 | * The new issue and new pull forms fit a 390 px phone in one column (#9719). | |
| 3 | * | |
| 4 | * There is no layout engine in the test run, so the fit is held on the real CSS the | |
| 5 | * page ships: every rule that can reach an element of the form (by one of its classes | |
| 6 | * or tags) is read, and none may fix a width or min-width over 390 px, nor lay the | |
| 7 | * fields side by side. The controls fill their field (`.input { width: 100% }`) and a | |
| 8 | * field is a block, so the form is as wide as the screen and no wider. A rule that | |
| 9 | * gave any of them a fixed width past 390 px would scroll the form sideways, and fails. | |
| 10 | */ | |
| 11 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 12 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 13 | import { tmpdir } from "node:os"; | |
| 14 | import { join } from "node:path"; | |
| 15 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 16 | import { createApp, type App } from "../src/app.ts"; | |
| 17 | import { createSession } from "../src/core/sessions.ts"; | |
| 18 | import { createUser } from "../src/core/users.ts"; | |
| 19 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 20 | import { utilities as base } from "../src/web/style/base.ts"; | |
| 21 | import { components } from "../src/web/style/components.ts"; | |
| 22 | import { content } from "../src/web/style/content.ts"; | |
| 23 | ||
| 24 | let dir: string; | |
| 25 | let app: App; | |
| 26 | let servers: Servers; | |
| 27 | let cookie: string; | |
| 28 | ||
| 29 | const must = <T>(r: { ok: true; value: T } | { ok: false; error: unknown }): T => { | |
| 30 | if (!r.ok) throw new Error(JSON.stringify(r.error)); | |
| 31 | return r.value; | |
| 32 | }; | |
| 33 | ||
| 34 | beforeAll(async () => { | |
| 35 | dir = await mkdtemp(join(tmpdir(), "forge-compose-fit-")); | |
| 36 | app = createApp({ dataDir: dir, port: 0 }); | |
| 37 | servers = await serve(app); | |
| 38 | const u = must(await createUser(app.db, { origin: "seed", trusted: true, username: "alice", email: "[email protected]", password: "a-good-passphrase", adminIfFirst: true })); | |
| 39 | const repo = must(await createRepo(app, u, { name: "fit", visibility: "public" })); | |
| 40 | must(await commitToBranch(app, u, repo, { message: "init", files: [{ path: "README.md", content: "x" }] })); | |
| 41 | must(await commitToBranch(app, u, repo, { branch: "topic", message: "c", files: [{ path: "t.txt", content: "x" }] })); | |
| 42 | cookie = `forge_session=${createSession(app.db, u.id).token}; forge_mode=dev`; | |
| 43 | }, 60_000); | |
| 44 | afterAll(async () => { | |
| 45 | servers.stop(); | |
| 46 | app.close(); | |
| 47 | await rm(dir, { recursive: true, force: true }); | |
| 48 | }); | |
| 49 | ||
| 50 | /** The compose form's markup and the page's own <style> blocks. */ | |
| 51 | async function compose(kind: "issues" | "pulls"): Promise<{ form: string; styles: string[] }> { | |
| 52 | const page = await (await fetch(`${servers.url}/alice/fit/${kind}/new`, { headers: { cookie } })).text(); | |
| 53 | const at = page.indexOf(`<form method="post" action="/alice/fit/${kind}/new"`); | |
| 54 | expect(at).toBeGreaterThan(-1); | |
| 55 | return { | |
| 56 | form: page.slice(at, page.indexOf("</form>", at) + 7), | |
| 57 | styles: page.match(/<style[^>]*>[\s\S]*?<\/style>/g) ?? [], | |
| 58 | }; | |
| 59 | } | |
| 60 | ||
| 61 | /** Each CSS rule as selector and body; an @media prelude stays on its first rule's selector. */ | |
| 62 | function rules(css: string): { selector: string; body: string }[] { | |
| 63 | return [...css.matchAll(/([^{}]+)\{([^{}]*)\}/g)].map((m) => ({ selector: m[1]!.trim(), body: m[2]! })); | |
| 64 | } | |
| 65 | ||
| 66 | /** Rules whose selector names a class or tag that appears in the form. */ | |
| 67 | function reaching(form: string, css: string[]): { selector: string; body: string }[] { | |
| 68 | const classes = new Set([...form.matchAll(/class="([^"]*)"/g)].flatMap((m) => m[1]!.split(/\s+/))); | |
| 69 | const tags = new Set([...form.matchAll(/<([a-z][a-z0-9]*)\b/g)].map((m) => m[1]!)); | |
| 70 | return css.flatMap(rules).filter(({ selector }) => { | |
| 71 | const sel = selector.replace(/@media[^{]*$/, ""); | |
| 72 | for (const m of sel.matchAll(/\.([\w-]+)/g)) if (classes.has(m[1]!)) return true; | |
| 73 | for (const m of sel.matchAll(/(?:^|[\s,>+~])([a-z][a-z0-9]*)(?=[\s,.:[>+~]|$)/g)) if (tags.has(m[1]!)) return true; | |
| 74 | return false; | |
| 75 | }); | |
| 76 | } | |
| 77 | ||
| 78 | const PX: Record<string, number> = { px: 1, rem: 16, em: 16, ch: 8 }; | |
| 79 | ||
| 80 | for (const kind of ["issues", "pulls"] as const) { | |
| 81 | describe(`the new ${kind === "issues" ? "issue" : "pull"} form at 390 px`, () => { | |
| 82 | test("no CSS rule that reaches the form fixes a width over 390 px", async () => { | |
| 83 | const { form, styles } = await compose(kind); | |
| 84 | expect(form).not.toMatch(/\sstyle="[^"]*width/i); | |
| 85 | const hit = reaching(form, [base, components, content, ...styles]); | |
| 86 | expect(hit.some((r) => /(^|[\s,])\.input\b/.test(r.selector))).toBe(true); | |
| 87 | const over: string[] = []; | |
| 88 | for (const { selector, body } of hit) { | |
| 89 | for (const m of body.matchAll(/(?<![-\w])(?:min-)?width\s*:\s*(-?[\d.]+)(px|rem|em|ch)\b/g)) { | |
| 90 | if (Number(m[1]) * PX[m[2]!]! > 390) over.push(`${selector} { ${m[0]} }`); | |
| 91 | } | |
| 92 | } | |
| 93 | expect(over).toEqual([]); | |
| 94 | }); | |
| 95 | ||
| 96 | test("the controls fill their field and the fields stack in one column", async () => { | |
| 97 | const { form } = await compose(kind); | |
| 98 | expect(components).toMatch(/(?:^|\n)\.input \{[^}]*\bwidth: 100%/); | |
| 99 | expect(components).toMatch(/(?:^|\n)\.field \{[^}]*display: block/); | |
| 100 | expect(form).not.toContain("field-row"); | |
| 101 | for (const m of form.matchAll(/<(select|textarea)\b[^>]*>/g)) expect(m[0]).toContain('class="input'); | |
| 102 | for (const m of form.matchAll(/<input\b[^>]*>/g)) { | |
| 103 | if (!/type="(hidden|checkbox)"/.test(m[0])) expect(m[0]).toContain('class="input'); | |
| 104 | } | |
| 105 | }); | |
| 106 | ||
| 107 | test("one button, and Cancel is the link variant", async () => { | |
| 108 | const { form } = await compose(kind); | |
| 109 | expect(form.match(/<button/g)?.length).toBe(1); | |
| 110 | expect(form).toMatch(/<a\b[^>]*data-variant="link"[^>]*>\s*Cancel/); | |
| 111 | }); | |
| 112 | }); | |
| 113 | } |
Principle 3: this bare
<div>has no class and does nothing. Remove it, and the one around Title/Description below, and let.fieldcarry the rhythm.