diff --git a/web/SitePreview.tsx b/web/SitePreview.tsx index 8acea7d..37b532c 100644 --- a/web/SitePreview.tsx +++ b/web/SitePreview.tsx @@ -66,6 +66,12 @@ export const SitePreview = ({ * * Return a cleanup and it runs before the next load and on unmount, so nothing injected outlives the * document it was injected into. + * + * ── Give it a stable identity, and change that identity to REPAINT ── + * + * It is re-run whenever it changes, so a `useCallback` whose dependencies are the data being drawn is + * exactly right: new data, new function, redrawn. An inline arrow is a new function every render and + * would reinject on every one. */ onDocument?: (document: Document) => void | (() => void); /** @@ -167,26 +173,28 @@ export const SitePreview = ({ }, [applyTheme, ready]); /* - Hand the loaded document to whoever asked for it. + Hand the loaded document to whoever asked for it — on load, and again whenever they change. - Through a ref so a caller's inline arrow — which is a new function every render — does not re-run - the injection on every render. Same trap as `useClient`, one level along. + Two things have to happen and they race: the frame fetches a new document, and the caller fetches + whatever it means to draw on it. Neither reliably finishes first. Running the injection ONLY on load + meant painting with whatever data had arrived by then, which after creating a section was usually + the data from before it existed. + + So `loaded` is state rather than a callback: the effect below depends on both it and `onDocument`, + and runs again when either moves. Whichever arrives second triggers the paint that counts. */ - const onDocumentRef = useRef(onDocument); - onDocumentRef.current = onDocument; - const cleanupRef = useRef<(() => void) | void>(undefined); - - const handOverDocument = useCallback(() => { - // Whatever was injected into the PREVIOUS document goes first. A reload replaces the document, so - // a cleanup that runs after the fact is cleaning something that no longer exists. - cleanupRef.current?.(); - cleanupRef.current = undefined; + const [loaded, setLoaded] = useState(0); + useEffect(() => { + if (!loaded) return; const frameDocument = frame.current?.contentDocument; - if (frameDocument) cleanupRef.current = onDocumentRef.current?.(frameDocument); - }, []); + if (!frameDocument) return; - useEffect(() => () => cleanupRef.current?.(), []); + const cleanup = onDocument?.(frameDocument); + // Runs before the next paint and on unmount, so nothing injected outlives its document — and two + // paints never leave two sets of controls. + return () => cleanup?.(); + }, [loaded, onDocument]); const message = !port ? 'No development server is configured — set its port under Administration.' @@ -236,12 +244,17 @@ export const SitePreview = ({ // The key is what makes `reloadToken` reload: a new key is a new element, so the browser // fetches the document again and `onLoad` runs, which re-injects the builder's chrome. key={reloadToken ?? 0} - ref={frame} + // A remount is a NEW document, and the effect above must not paint into the old one on + // the strength of a `loaded` that referred to it. + ref={(node) => { + frame.current = node; + if (!node) setLoaded(0); + }} // Applied on load as well as on change: the document is replaced on a reload, and the // properties go with it. onLoad={() => { applyTheme(); - handOverDocument(); + setLoaded((n) => n + 1); }} src={`/api/dev-proxy/${port}/${path}`} title={`The site at /${path}`} diff --git a/web/canvas.ts b/web/canvas.ts index 221713a..bfc541c 100644 --- a/web/canvas.ts +++ b/web/canvas.ts @@ -9,40 +9,58 @@ * * It is possible because the preview frame is SAME ORIGIN — it comes through the platform's dev proxy * on Officer's own host — so `iframe.contentDocument` is readable and writable. That single fact is - * what the whole page builder rests on, and it is why swapping this plugin's hand-rolled cross-origin - * proxy for the platform's mattered beyond the bugs it fixed. + * what the whole page builder rests on. + * + * ── What the SITE has to provide, and it is only this ── + * + * data-page-empty the placeholder shown when a page has no sections + * data-section-id which row a band came from + * data-column a column within a band + * + * Three attributes and no behaviour. Markers rather than classes, so they cannot be mistaken for + * styling, cannot be purged by a build that has never heard of this file, and read as what they are: + * handles for something else. * * ── Plain DOM, not React ── * * React could portal into another document, and it would mean running a reconciler against a document - * that reloads under it whenever the dev server rebuilds. The controls here are a handful of elements - * with click handlers; the cost of building them by hand is a few lines, and the benefit is that a - * reload is just a document that no longer exists rather than a tree React still thinks it owns. + * that reloads under it whenever the dev server rebuilds. These are a handful of elements with click + * handlers; building them by hand costs a few lines, and a reload becomes a document that no longer + * exists rather than a tree React still thinks it owns. * * ── Styles are inline, on purpose ── * * The frame has the SITE's stylesheet, which is Tailwind built by scanning the site's own templates. * A class this file invents was never scanned and does not exist there, so it would silently do - * nothing. Inline styles cannot be purged by a build that has never heard of this file. + * nothing. */ /** Everything injected carries this, so a re-injection can find and remove what the last one left. */ const MARK = 'data-fofos-admin-canvas'; +/** The editor's own colour. Deliberately not the site's — this is chrome, and it should not blend in. */ +const ACCENT = '#3b82f6'; + +export type CanvasSection = { id: number; name: string; layout: 'one' | 'two' }; + export type Canvas = { - /** Somebody pressed Add Section. */ + sections: CanvasSection[]; onAddSection: () => void; + onDeleteSection: (section: CanvasSection) => void; }; /** * Where the page's content goes. * * `
` because it is what the site's Layout renders and what the word means — the page minus the - * furniture around it. Not a class or a data attribute, which would be a contract the SITE has to keep, - * and the point of this file is that the site does not have to know it exists. + * furniture around it. */ const contentRoot = (document: Document): HTMLElement | null => document.querySelector('main'); +const style = (element: HTMLElement, rules: string[]) => { + element.style.cssText = rules.join(';'); +}; + /** * Draw the builder's controls into a rendered page. * @@ -54,58 +72,161 @@ export function paintCanvas(document: Document, canvas: Canvas): () => void { if (!root) return () => {}; // Anything a previous injection left. Cheap insurance: the cleanup should have run, and if a reload - // raced it, a second Add Section button is exactly the sort of thing nobody reports as a bug. + // raced it, a second Add section button is exactly the sort of thing nobody reports as a bug. root.querySelectorAll(`[${MARK}]`).forEach((node) => node.remove()); + /** Everything to undo, in the order it was done. */ + const undo: (() => void)[] = []; + /* - The site's EMPTY-PAGE placeholder, hidden while the builder draws its own controls in that space. + The site's empty-page placeholder, hidden while the builder draws its own controls in that space. - Found by its marker rather than by position. It was `firstElementChild`, which was right for exactly - as long as an empty page was the only page — the moment a page has a section, the first child IS - that section and hiding it would hide the thing being edited. - - Hidden rather than removed, so the cleanup can simply put it back. A page whose last section is - deleted then shows it again without a reload. + Found by its marker rather than by position. Hidden rather than removed, so the cleanup puts it + back — a page whose last section is deleted shows it again without a reload. */ const placeholder = root.querySelector('[data-page-empty]'); - if (placeholder) placeholder.style.display = 'none'; + if (placeholder) { + placeholder.style.display = 'none'; + undo.push(() => { + placeholder.style.display = ''; + }); + } + /* + Every section, outlined and named. + + This is the whole reason the decoration exists: an EMPTY section renders as nothing, which is + correct on the public site and useless in an editor. Somebody who has just chosen "two columns" + needs to see two columns, not a blank page they have to take on trust. + */ + for (const section of canvas.sections) { + const element = root.querySelector(`[data-section-id="${section.id}"]`); + if (!element) continue; + + const previousPosition = element.style.position; + const previousOutline = element.style.outline; + const previousOffset = element.style.outlineOffset; + const previousMargin = element.style.margin; + + // `relative` so the label and the controls can be placed against this band rather than the page. + element.style.position = 'relative'; + element.style.outline = `1px solid ${ACCENT}`; + element.style.outlineOffset = '4px'; + // Room for the outline and the label above it, so two bands in a row do not touch. + element.style.margin = '2.5rem 0'; + + undo.push(() => { + element.style.position = previousPosition; + element.style.outline = previousOutline; + element.style.outlineOffset = previousOffset; + element.style.margin = previousMargin; + }); + + const bar = document.createElement('div'); + bar.setAttribute(MARK, 'section-bar'); + style(bar, [ + 'position:absolute', + 'top:-1.75rem', + 'left:0', + 'right:0', + 'display:flex', + 'align-items:center', + 'gap:0.5rem', + 'font-size:0.6875rem', + `color:${ACCENT}`, + // The page's own font would make this look like content. It is not content. + 'font-family:ui-sans-serif, system-ui, sans-serif', + ]); + + const label = document.createElement('span'); + label.textContent = section.name; + style(label, ['font-weight:600']); + + const shape = document.createElement('span'); + shape.textContent = section.layout === 'two' ? 'two columns' : 'one column'; + style(shape, ['opacity:0.7']); + + const remove = document.createElement('button'); + remove.type = 'button'; + remove.textContent = 'Delete'; + style(remove, [ + 'margin-left:auto', + 'cursor:pointer', + 'background:none', + 'border:0', + 'padding:0', + 'font:inherit', + 'color:inherit', + 'opacity:0.7', + 'text-decoration:underline', + ]); + const onRemove = () => canvas.onDeleteSection(section); + remove.addEventListener('click', onRemove); + undo.push(() => remove.removeEventListener('click', onRemove)); + + bar.append(label, shape, remove); + element.append(bar); + undo.push(() => bar.remove()); + + /* + The columns, outlined too. + + An empty column has height and nothing else, so a two-column band and a one-column band look + identical until something is in them — which is exactly when it is too late to notice you chose + the wrong one. + */ + element.querySelectorAll('[data-column]').forEach((column) => { + const was = column.style.outline; + const wasOffset = column.style.outlineOffset; + column.style.outline = `1px dashed color-mix(in srgb, ${ACCENT} 45%, transparent)`; + column.style.outlineOffset = '-4px'; + undo.push(() => { + column.style.outline = was; + column.style.outlineOffset = wasOffset; + }); + }); + } + + // ── Add section, always last ───────────────────────────────────────────────────────────────── const zone = document.createElement('div'); zone.setAttribute(MARK, 'add-section'); - zone.style.cssText = [ + style(zone, [ 'display:flex', 'align-items:center', 'justify-content:center', - 'min-height:40vh', - 'margin:2rem 0', - // A dashed outline reads as "a space waiting for something" in every editor anybody has used. + // Tall when the page is empty, modest once there is something to add to — the zone should not + // dwarf the content it sits under. + canvas.sections.length ? 'min-height:8rem' : 'min-height:40vh', + 'margin:2.5rem 0', 'border:2px dashed color-mix(in srgb, currentColor 25%, transparent)', 'border-radius:0.75rem', - ].join(';'); + ]); const button = document.createElement('button'); button.type = 'button'; button.textContent = '+ Add section'; - button.style.cssText = [ + style(button, [ 'cursor:pointer', 'padding:0.75rem 1.5rem', 'border-radius:0.5rem', - // The page's own colours, inverted — so the control belongs to whatever theme the site is wearing - // rather than being a grey box sitting on it. + // The page's own colours, inverted, so the control belongs to whatever theme the site is wearing. 'background:var(--site-fg, #000)', 'color:var(--site-bg, #fff)', 'border:0', 'font:inherit', 'font-size:0.875rem', - ].join(';'); + ]); button.addEventListener('click', canvas.onAddSection); zone.append(button); root.append(zone); - - return () => { + undo.push(() => { button.removeEventListener('click', canvas.onAddSection); zone.remove(); - if (placeholder) placeholder.style.display = ''; + }); + + return () => { + // Reversed, so a style put back is put back over the value that replaced it rather than under. + for (const step of undo.reverse()) step(); }; } diff --git a/web/sections/PageBuilderPanel.tsx b/web/sections/PageBuilderPanel.tsx index dca69ac..a90b8b6 100644 --- a/web/sections/PageBuilderPanel.tsx +++ b/web/sections/PageBuilderPanel.tsx @@ -3,7 +3,7 @@ import { useSearchParams } from 'react-router'; import { useClient } from 'hooks/useClient'; import { readableError } from '../errors'; import { SitePreview, useDevPort } from '../SitePreview'; -import { paintCanvas } from '../canvas'; +import { paintCanvas, type CanvasSection } from '../canvas'; import { AddSectionDialog, type SectionLayout } from './AddSectionDialog'; import { usePages, type Page } from '../usePages'; @@ -34,6 +34,17 @@ export const PageBuilderPanel = () => { const { pages, announce } = usePages(); const [error, setError] = useState(null); + const [sections, setSections] = useState([]); + + /* + The handlers, through a ref; the DATA, as a dependency. + + `onDocument` is re-run whenever its identity changes, so what it should depend on is the thing being + drawn — the sections — and nothing else. The callbacks are ordinary functions rebuilt every render, + so putting them in the dependency list would repaint on every render; through a ref they are always + the current ones without being part of the identity. + */ + const handlersRef = useRef<{ add: () => void; remove: (id: number) => void }>({ add: () => {}, remove: () => {} }); const [adding, setAdding] = useState(false); const [creating, setCreating] = useState(false); /* @@ -52,10 +63,34 @@ export const PageBuilderPanel = () => { be a new injection each render. */ const onDocument = useCallback( - (frameDocument: Document) => paintCanvas(frameDocument, { onAddSection: () => setAdding(true) }), - [], + (frameDocument: Document) => + paintCanvas(frameDocument, { + sections, + onAddSection: () => handlersRef.current.add(), + onDeleteSection: (section) => handlersRef.current.remove(section.id), + }), + [sections], ); + /* + The handlers, through a ref; the DATA, as a dependency. + + `onDocument` is re-run whenever its identity changes, so what it should depend on is the thing being + drawn — the sections — and nothing else. The callbacks are ordinary functions rebuilt every render, + so putting them in the dependency list would repaint on every render; through a ref they are always + the current ones without being part of the identity. + */ + const deleteSection = async (sectionId: number) => { + if (!page) return; + setError(null); + try { + await clientRef.current.delete(`/fofos-admin/pages/${page.id}/sections/${sectionId}`); + setRendered((n) => n + 1); + } catch (failure) { + setError(readableError(failure)); + } + }; + const createSection = async (values: { name: string; layout: SectionLayout }) => { if (!page) return; setCreating(true); @@ -89,6 +124,25 @@ export const PageBuilderPanel = () => { const page = pages?.find((row) => row.id === id) ?? null; + /* + The page's sections, which the canvas needs to name and outline what it finds in the document. + + Fetched here rather than read out of the DOM. The markup says which id a band came from and nothing + else — no name, no layout — and inferring the layout from the classes would be the admin reading the + site's stylesheet, which is exactly the coupling all of this avoids. + */ + useEffect(() => { + if (!page) return; + void clientRef.current + .get<{ data: CanvasSection[] }>(`/fofos-admin/pages/${page.id}/sections`) + .then((response) => setSections(response.data)) + .catch((failure: unknown) => setError(readableError(failure))); + }, [page, rendered]); + + // Assigned on every render, after both functions above exist, so the ref always holds the current + // pair without either being part of `onDocument`'s identity. + handlersRef.current = { add: () => setAdding(true), remove: (sectionId: number) => void deleteSection(sectionId) }; + if (error) return
{error}
; if (!pages) return
Loading…
; @@ -107,6 +161,8 @@ export const PageBuilderPanel = () => { {/* The real page, at its own address, with the builder's own controls drawn inside it. `path` has no leading slash — Home's is the empty string, which the proxy resolves to the root. */} + {/* `reloadToken` moves on every write, which refetches the document AND re-runs `onDocument` — + so the decoration is redrawn against the sections that now exist. */}