Public: anyone can read this; the crew can change it.
The /bring steps speak the home page's language #9774
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9712-the-bring-steps-speak-the-home-page-s-la → main 3 commits fork 2 threads · 2 unresolved
2 files
changed +34
−16
src/web/pages/bring.ts +17 −14
| @@ -18,11 +18,15 @@ import type { Flash } from "../flash.ts"; | ||
| 18 | 18 | import { html, raw, type Html } from "../html.ts"; |
| 19 | 19 | import { page, redirect } from "../layout.ts"; |
| 20 | 20 | import { mirrorJs } from "../client.ts"; |
| 21 | import { btn, card, field, icon } from "../ui.ts"; | |
| 21 | import { btn, field } from "../ui.ts"; | |
| 22 | 22 | import type { Ctx } from "../../http/context.ts"; |
| 23 | 23 | import { accountOnly, hostLabel } from "./origins.ts"; |
| 24 | 24 | import { stageList } from "./browse.ts"; |
| 25 | 25 | |
| 26 | /** One step of the page: a heading over its words and form, no box around them. No heading when the page's h1 already says it. */ | |
| 27 | const step = (heading: Html | null, body: Html, id?: string): Html => | |
| 28 | html`<section class="stack" ${id === undefined ? null : html`id="${id}"`}>${heading === null ? null : html`<h2>${heading}</h2>`}${body}</section>`; | |
| 29 | ||
| 26 | 30 | const NOT_COPIED = ["data", "variables", "secrets"]; |
| 27 | 31 | |
| 28 | 32 | /** The card's address: `http:` only when the link says so (the card then refuses a token unless this is a development forge). */ |
| @@ -35,7 +39,8 @@ const cardSite = (host: string, link: string): string => `${/^http:\/\//i.test(l | ||
| 35 | 39 | */ |
| 36 | 40 | function connectCard(host: string, link: string, error?: string): Html { |
| 37 | 41 | const site = cardSite(host, link); |
| 38 | return card( | |
| 42 | return step( | |
| 43 | html`Connect ${hostLabel(site)} first`, | |
| 39 | 44 | html`<p> |
| 40 | 45 | This app is on <strong>${hostLabel(site)}</strong>, and you have not |
| 41 | 46 | connected your account there yet. Do it here: your link is kept. |
| @@ -68,7 +73,7 @@ function connectCard(host: string, link: string, error?: string): Html { | ||
| 68 | 73 | <div class="form-actions">${btn("Connect and bring it", { icon: "download" })}</div> |
| 69 | 74 | </form> |
| 70 | 75 | </details>`, |
| 71 | { id: "bring-connect-card", head: html`${icon("globe")}<h2>Connect ${hostLabel(site)} first</h2>` }, | |
| 76 | "bring-connect-card", | |
| 72 | 77 | ); |
| 73 | 78 | } |
| 74 | 79 | |
| @@ -80,7 +85,8 @@ function form(ctx: Ctx, value: string, problem?: Flash, status = 200, connect?: | ||
| 80 | 85 | ...(problem === undefined ? {} : { flash: problem }), |
| 81 | 86 | body: html`<div class="page-head"><h1>Bring an app</h1></div> |
| 82 | 87 | <div class="narrow-wide"> |
| 83 | ${card( | |
| 88 | ${step( | |
| 89 | null, | |
| 84 | 90 | html`<p class="muted small"> |
| 85 | 91 | Paste the link to an app on another forge. It comes across with |
| 86 | 92 | how it builds and runs, and goes live here at its own address. |
| @@ -101,7 +107,6 @@ function form(ctx: Ctx, value: string, problem?: Flash, status = 200, connect?: | ||
| 101 | 107 | })} |
| 102 | 108 | <div class="bar">${btn("Bring it here", { ...(connect === undefined ? { variant: "primary" as const } : {}), icon: "download" })}</div> |
| 103 | 109 | </form>`, |
| 104 | { head: html`${icon("download")}<h2>Bring it here</h2>` }, | |
| 105 | 110 | )} |
| 106 | 111 | ${connect ?? null} |
| 107 | 112 | </div>`, |
| @@ -201,7 +206,8 @@ export async function bringJobPage(ctx: Ctx): Promise<Response> { | ||
| 201 | 206 | title: `Brought ${job.fullName}`, |
| 202 | 207 | body: html`<div class="page-head"><h1>${job.fullName} is here</h1></div> |
| 203 | 208 | <div class="narrow-wide"> |
| 204 | ${card( | |
| 209 | ${step( | |
| 210 | html`Ready to run`, | |
| 205 | 211 | html`<p> |
| 206 | 212 | The code and its history are copied, linked back to |
| 207 | 213 | <span class="mono">${job.fullName}</span> so your improvements |
| @@ -211,7 +217,6 @@ export async function bringJobPage(ctx: Ctx): Promise<Response> { | ||
| 211 | 217 | <form method="post" action="${here}/run" class="bar"> |
| 212 | 218 | ${btn("Run it here", { variant: "primary", icon: "rocket" })} |
| 213 | 219 | </form>`, |
| 214 | { head: html`${icon("rocket")}<h2>Ready to run</h2>` }, | |
| 215 | 220 | )} |
| 216 | 221 | </div>`, |
| 217 | 222 | }); |
| @@ -221,18 +226,19 @@ export async function bringJobPage(ctx: Ctx): Promise<Response> { | ||
| 221 | 226 | title: `Brought ${job.fullName}`, |
| 222 | 227 | body: html`<div class="page-head"><h1>${job.fullName} is here</h1></div> |
| 223 | 228 | <div class="narrow-wide"> |
| 224 | ${card( | |
| 229 | ${step( | |
| 230 | html`Live here`, | |
| 225 | 231 | html`<p> |
| 226 | 232 | It is deploying now as a private app, linked back to |
| 227 | 233 | <span class="mono">${job.fullName}</span> so your improvements |
| 228 | 234 | can flow to its author. Watch the first deploy on its page. |
| 229 | 235 | </p> |
| 230 | 236 | <div class="bar">${btn("Watch the first deploy", { href: open, variant: "primary", icon: "rocket" })}</div>`, |
| 231 | { head: html`${icon("rocket")}<h2>Live here</h2>` }, | |
| 232 | 237 | )} |
| 233 | 238 | ${asking.length === 0 |
| 234 | 239 | ? null |
| 235 | : card( | |
| 240 | : step( | |
| 241 | html`What it needs`, | |
| 236 | 242 | html`<p class="muted small"> |
| 237 | 243 | The app lists these in its <span class="mono">.env.example</span>. |
| 238 | 244 | Nothing came with it: give what you want it to have, leave the |
| @@ -244,7 +250,6 @@ export async function bringJobPage(ctx: Ctx): Promise<Response> { | ||
| 244 | 250 | )} |
| 245 | 251 | <div class="bar">${btn("Save and continue", { variant: "primary", icon: "key" })}</div> |
| 246 | 252 | </form>`, |
| 247 | { head: html`${icon("key")}<h2>What it needs</h2>` }, | |
| 248 | 253 | )} |
| 249 | 254 | </div>`, |
| 250 | 255 | }); |
| @@ -257,9 +262,7 @@ export async function bringJobPage(ctx: Ctx): Promise<Response> { | ||
| 257 | 262 | title: `Bringing ${job.fullName}`, |
| 258 | 263 | body: html`<div class="page-head"><h1>Bringing ${job.fullName}</h1></div> |
| 259 | 264 | <div class="narrow-wide"> |
| 260 | ${card(stageList(job, running ? stream : "", here), { | |
| 261 | head: html`${icon("download")}<h2>${job.fullName}</h2>`, | |
| 262 | })} | |
| 265 | ${step(null, stageList(job, running ? stream : "", here))} | |
outdated
Principle 1: the h1 already reads "Bringing {fullName}", and this h2 repeats the same name over the stage list. Use a heading that adds something (e.g. "Progress") or omit it, and adjust the step helper to allow no heading. |
||
| 263 | 266 | ${job.stage === "failed" |
| 264 | 267 | ? html`<p class="mt" role="alert">${job.error ?? "The import failed."}</p> |
| 265 | 268 | <div class="bar mt">${btn("Try another link", { href: "/bring", variant: "outline" })}</div>` |
test/bring.test.ts +17 −2
| @@ -74,6 +74,14 @@ const PASSWORD = "alice-secret-7089"; | ||
| 74 | 74 | const basic = (user: string, secret: string): string => |
| 75 | 75 | `Basic ${Buffer.from(`${user}:${secret}`).toString("base64")}`; |
| 76 | 76 | |
| 77 | /** Rows, not boxes (#9712): a step is a heading over its words and form, never a card. */ | |
| 78 | const rowsNotBoxes = (text: string, form: RegExp, heading: string | null): void => { | |
| 79 | expect(text).not.toMatch(/class="card[\s"]/); | |
| 80 | expect(text).not.toContain("card-head"); | |
| 81 | expect(text).not.toContain("card-body"); | |
| 82 | expect(text).toMatch(new RegExp(`<section class="stack"[^>]*>${heading === null ? "(?!<h2>)" : `<h2>${heading}</h2>`}[\\s\\S]*?${form.source}`)); | |
| 83 | }; | |
| 84 | ||
| 77 | 85 | describe("bringing an app whole", () => { |
| 78 | 86 | let dirs: string[] = []; |
| 79 | 87 | let app: App; |
| @@ -149,7 +157,9 @@ describe("bringing an app whole", () => { | ||
| 149 | 157 | test("the page asks for a link", async () => { |
| 150 | 158 | const res = await fetch(`${servers.url}/bring`, { headers: { authorization: basic("alice", PASSWORD) } }); |
| 151 | 159 | expect(res.status).toBe(200); |
| 152 | expect(await res.text()).toContain('name="link"'); | |
| 160 | const text = await res.text(); | |
| 161 | expect(text).toContain('name="link"'); | |
| 162 | rowsNotBoxes(text, /<form method="post" action="\/bring"/, null); | |
| 153 | 163 | }); |
| 154 | 164 | |
| 155 | 165 | test("a bad link and an unconnected forge are refused in words, starting nothing", async () => { |
| @@ -163,6 +173,8 @@ describe("bringing an app whole", () => { | ||
| 163 | 173 | expect(card).toContain(`value="${fake.url}/octo/widgets"`); |
| 164 | 174 | expect(card).toContain(`${fake.url}/settings/tokens`); |
| 165 | 175 | expect(card).toContain("Sign in there"); |
| 176 | rowsNotBoxes(card, /action="\/settings\/origins\/oauth"/, "Connect [^<]* first"); | |
| 177 | expect(card).toContain('id="bring-connect-card"'); | |
| 166 | 178 | // The sign-in carries the link through and back. |
| 167 | 179 | expect(card).toContain('name="then" value="bring"'); |
| 168 | 180 | // A token the forge refuses keeps the card and the link, and echoes nothing. |
| @@ -203,7 +215,9 @@ describe("bringing an app whole", () => { | ||
| 203 | 215 | const get = () => fetch(`${servers.url}${where}`, { headers: { authorization: basic("alice", PASSWORD) } }); |
| 204 | 216 | const before = await get(); |
| 205 | 217 | expect(before.status).toBe(200); |
| 206 | expect(await before.text()).toContain(`${where}/run`); | |
| 218 | const beforeText = await before.text(); | |
| 219 | expect(beforeText).toContain(`${where}/run`); | |
| 220 | rowsNotBoxes(beforeText, new RegExp(`action="${where}/run"`), "Ready to run"); | |
| 207 | 221 | expect(getAppByRepo(app.db, getRepo(app.db, "alice", "widgets")!.id)).toBeUndefined(); |
| 208 | 222 | |
| 209 | 223 | const ran = await post(`${where}/run`, {}); |
| @@ -214,6 +228,7 @@ describe("bringing an app whole", () => { | ||
| 214 | 228 | const text = await landed.text(); |
| 215 | 229 | expect(text).toContain("/alice/widgets/app"); |
| 216 | 230 | expect(text).toContain("API_KEY"); |
| 231 | rowsNotBoxes(text, new RegExp(`action="${where}/env"`), "What it needs"); | |
| 217 | 232 | expect(text).toContain("GREETING"); |
| 218 | 233 | expect(text).not.toContain("real-secret"); |
| 219 | 234 |
Principle 1: the page says "Bring an app" (h1), then this h2 "Bring it here", then a button "Bring it here". Three times is once too many. Drop this step heading (render the link form with no h2, or let the h1 carry it) and keep the button as the one action. Update the test regex that expects this h2.