From 1df520c95d8342447eeda6d40a7da825e60489c0 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 08:18:12 +0200 Subject: [PATCH] chore: remove QuestionEditModal Nothing had imported it since editing moved to the full page; the only thing that still referred to it was its own test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- docs/TODO.md | 9 +- frontend/src/App.jsx | 8 +- frontend/src/components/ArticleReader.jsx | 386 +++++++++++++----- frontend/src/components/ArticleSaveButton.css | 45 ++ frontend/src/components/ArticleSaveButton.jsx | 155 +++++++ frontend/src/components/ArticleSplitPane.jsx | 25 +- frontend/src/components/QuestionEditors.jsx | 203 --------- .../src/components/QuestionEditors.test.jsx | 62 --- frontend/src/components/RichText.jsx | 17 + frontend/src/context/SessionChrome.jsx | 43 +- frontend/src/pages/ArticlesPage.jsx | 195 +++++---- 11 files changed, 673 insertions(+), 475 deletions(-) create mode 100644 frontend/src/components/ArticleSaveButton.css create mode 100644 frontend/src/components/ArticleSaveButton.jsx delete mode 100644 frontend/src/components/QuestionEditors.jsx delete mode 100644 frontend/src/components/QuestionEditors.test.jsx diff --git a/docs/TODO.md b/docs/TODO.md index 50e54f4..e3fd07d 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -271,13 +271,8 @@ Captured so nothing is lost while the article writing runs. MediaAsset already stores one path per image, so a `thumb_path` beside it plus a Caddy cache rule is the shape. -- [ ] **QuestionEditModal is dead code** — nothing has imported it since Edit - moved to the full page. Its sibling CreateQuestionModal was removed when - its last two callers were replaced; this one was already unreferenced, so - it is left for a deliberate decision rather than swept up. - -## Quiz runner - +- [x] **QuestionEditModal is dead code** — removed 2026-09-12, with its + test. Nothing had imported it since Edit moved to the full page. - [x] **Per-question notes** — done. The notes themselves were already built (`question_notes`, saved on blur); what was missing was removing the global notes tab that floated over the same screen, so it was never clear diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index aedde67..30cda11 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -2,7 +2,7 @@ import { Suspense, useEffect, useState } from 'react' import { BrowserRouter, Routes, Route, Navigate, Outlet, Link, useLocation, useParams } from 'react-router-dom' import { AuthProvider, useAuth } from './context/AuthContext' import { SessionDrawerProvider } from './context/SessionDrawer' -import { SessionChromeProvider, useInSession } from './context/SessionChrome' +import { SessionChromeProvider, useFullBleed, useInSession } from './context/SessionChrome' import { ThemeProvider } from './context/ThemeContext' import Navbar from './components/Navbar' import SiteFooter from './components/SiteFooter' @@ -65,6 +65,7 @@ function AppLayout() { const location = useLocation() const [searching, setSearching] = useState(false) const inSession = useInSession() + const fullBleed = useFullBleed() useEffect(() => { const onKey = (event) => { @@ -98,8 +99,9 @@ function AppLayout() { {/* A session takes the window. The page's own gutters and 1200px cap are for reading; a player is a fixed-height box whose columns scroll inside it, and every pixel the container reserves is a pixel the - question does not get. */} -
+ question does not get. An article asks for the same width without + the rest of the session chrome — hence two classes rather than one. */} +
diff --git a/frontend/src/components/ArticleReader.jsx b/frontend/src/components/ArticleReader.jsx index cac5b50..16cfee5 100644 --- a/frontend/src/components/ArticleReader.jsx +++ b/frontend/src/components/ArticleReader.jsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react' +import { useEffect, useLayoutEffect, useRef, useState } from 'react' import RichText from './RichText' /** * Article prose goes through the same renderer as everything else. @@ -15,9 +15,17 @@ export function Markdown({ children, attemptId }) { * An article as it reads: contents rail, summary, and sections that expand * where they sit. * - * One renderer serves both the page and the second pane beside it, so a - * cross-reference opened in split view behaves like the article it came from - * rather than like a cut-down copy of it. + * The reading page hands the window to this component — the rail meets the + * left edge and the prose takes everything to the right of it. Capping the + * page at 1080px and centring it left a thin ribbon of text in the middle of a + * wide screen with the rail floating somewhere in the margin, which is the + * complaint this layout answers. + * + * `bare` is the second article read beside the first. That pane used to render + * the whole of this component, rail and all, inside a column half the width of + * the page: two contents lists and two scrollbars for one paragraph of prose, + * each cross-reference pushing the text further right. A pane is a column to + * read, not a page to navigate, so it gets the prose and nothing else. * * `activeSection` is the caller's, because the page keeps it in the URL for deep * links and the pane keeps it to itself; everything else — what is open, the @@ -36,17 +44,77 @@ const VIEWS = [ { key: 'clinical', label: 'Clinical' }, ] -export default function ArticleReader({ article, activeSection = '', onOpenSection, idPrefix = '', landmark = true, children }) { +// Remembered rather than reset on every article, the way the session player +// remembers its own rail: hiding the contents is a statement about how you +// read, not about the page you happened to be on when you said it. +const RAIL_KEY = 'pedshub.articleRail' +const SIZE_KEY = 'pedshub.articleText' +const SIZES = ['s', 'm', 'l'] + +const stored = (key, fallback) => { + try { return localStorage.getItem(key) ?? fallback } catch { return fallback } +} +const remember = (key, value) => { + try { localStorage.setItem(key, value) } catch { /* private browsing: the choice lasts the visit */ } +} + +/** + * How far down the window the sticky furniture has to start. + * + * Measured off the header rather than written down as a number, because the + * navbar's section strip collapses as you scroll and a hard-coded offset would + * either leave a band of page showing above the rail or hide the first line of + * it behind the bar. A second copy of the navbar's scroll logic would only be + * a guess about somebody else's component; its height is the fact itself. + */ +function useHeaderOffset() { + const [top, setTop] = useState(0) + useLayoutEffect(() => { + if (typeof document === 'undefined') return undefined + const bar = document.querySelector('.navbar') + if (!bar) return undefined + const measure = () => setTop(Math.round(bar.getBoundingClientRect().height)) + measure() + if (typeof ResizeObserver === 'undefined') return undefined + const observer = new ResizeObserver(measure) + observer.observe(bar) + return () => observer.disconnect() + }, []) + return top +} + +export default function ArticleReader({ + article, activeSection = '', onOpenSection, idPrefix = '', landmark = true, + // No rail, no bar, no chrome: the reader used as a second column. + bare = false, + // The trail back to the library, and the article's own icon toolbar. Owned + // by the page because publishing and editing are the page's business, drawn + // here because they belong above the title rather than above the layout. + breadcrumbs = null, toolbar = null, + // A second column to the right of the prose, inside the same grid — so the + // rail stays flush left and the pane can never push the article sideways. + aside = null, + children, +}) { // Which sections are open. Everything starts closed: an article is a reference // you consult, and a wall of prose hides the one heading you came for. const [openIds, setOpenIds] = useState({}) const [drawerOpen, setDrawerOpen] = useState(false) // Collapsing the contents rail hands its width to the prose. - const [railOpen, setRailOpen] = useState(true) + const [railOpen, setRailOpen] = useState(() => stored(RAIL_KEY, 'open') !== 'closed') + const [size, setSize] = useState(() => (SIZES.includes(stored(SIZE_KEY, 'm')) ? stored(SIZE_KEY, 'm') : 'm')) + // The section under the reader's eye, which is not the same thing as the one + // they last clicked: the bar names where you are, so it has to follow the + // scroll rather than the last deep link. + const [reading, setReading] = useState('') const root = useRef(null) + const headerTop = useHeaderOffset() // A page has one main landmark, so the second reader on it is not one. const Content = landmark ? 'main' : 'div' + useEffect(() => { remember(RAIL_KEY, railOpen ? 'open' : 'closed') }, [railOpen]) + useEffect(() => { remember(SIZE_KEY, size) }, [size]) + // Older articles have no variant on their sections; they are the full article. const everySection = (article.sections || []).map( sec => ({ ...sec, variant: sec.variant || 'long' })) @@ -59,10 +127,12 @@ export default function ArticleReader({ article, activeSection = '', onOpenSecti // space — which read as the view having no content at all. useEffect(() => { const tops = allSections.filter(sec => !sec.parent_id) - if (tops.length === 1) { + // A pane has no contents list to navigate by, so a wall of chevrons there + // is a wall of dead ends: the cross-reference opens as prose. + if (bare || tops.length === 1) { setOpenIds(Object.fromEntries(allSections.map(sec => [sec.id, true]))) } - }, [view, article]) + }, [view, article, bare]) // Element ids are prefixed because two readers can share a page, and a // duplicate id would point the pane's contents at the article behind it. const sectionElement = (secId) => root.current?.querySelector(`#section-${idPrefix}${secId}`) @@ -108,12 +178,40 @@ export default function ArticleReader({ article, activeSection = '', onOpenSecti // shown as prose rather than as a contents page of one entry. const soleSection = topSections.length === 1 && allSections.length === 1 ? topSections[0] : null + /** + * Which heading the reader is under. + * + * The bar names it once the title has scrolled away, and the rail marks the + * same one — a rail pointing at a heading you left three screens ago is + * worse than a rail pointing at nothing. + */ + const openKey = Object.keys(openIds).filter(id => openIds[id]).sort().join(',') + useEffect(() => { + if (bare || typeof IntersectionObserver === 'undefined') return undefined + const nodes = [...(root.current?.querySelectorAll('[data-section-id]') || [])] + if (!nodes.length) return undefined + const seen = new Map() + const observer = new IntersectionObserver(entries => { + entries.forEach(entry => seen.set(entry.target, entry)) + const live = [...seen.values()].filter(entry => entry.isIntersecting) + .sort((a, b) => a.boundingClientRect.top - b.boundingClientRect.top) + // Nothing in the band means the reader is above the first heading or + // below the last; either way the previous answer is still the best one. + if (live.length) setReading(live[0].target.dataset.sectionId) + }, { rootMargin: `-${headerTop + 56}px 0px -62% 0px` }) + nodes.forEach(node => observer.observe(node)) + return () => observer.disconnect() + }, [article, view, openKey, headerTop, bare]) + + const marked = reading || activeSection + const readingTitle = allSections.find(sec => sec.id === reading)?.title || '' + const renderSection = (sec, depth) => { const isOpen = !!openIds[sec.id] const kids = kidsOf(sec.id) const Heading = depth === 0 ? 'h2' : 'h3' return ( -
-
+ ) + + return ( +
+ - + {/* Where the rail was, so putting it back is where you last saw it. */} + {!railOpen && ( + + )} + +
+
{breadcrumbs}
+
+ {toolbar} + {/* Three steps, not a slider: a reader wants bigger or smaller, + and a control with twelve answers to that is a control you + have to think about mid-paragraph. */} + +
+
{article.updated_at && (

Last edited {new Date(article.updated_at).toLocaleDateString(undefined, { year: 'numeric', month: 'short', day: 'numeric' })}

)} - {article.summary &&

{article.summary}

} - {article.content && {article.content}} - {!allSections.length && (!article.summary && !article.content) && ( -
Content is being prepared by educators.
- )} +

+ {article.title} + {/* Drawn here rather than passed in, because a draft badge belongs + to the title and the title now lives with the reader. */} + {article.status && article.status !== 'published' && ( + Draft + )} +

{/* Headings first, prose on request: the article opens as a contents page you can scan, and each section expands where it sits rather - than in a modal that loses the thread. */} + than in a modal that loses the thread. The row sticks under the + header, so the controls stay put and the trail can say where in the + article you have got to once the title has gone. */}
- {/* Only the views this article actually has: an empty tab is a - promise the article cannot keep. */} - {present.length > 1 && ( -
- {present.map(option => ( - - ))} -
- )} - {allSections.length > 0 && !soleSection && ( - - )} + {readingTitle && ( + + {article.title} + + {readingTitle} + + )} +
+
+ {allSections.length > 0 && !soleSection && ( + + )} + {/* Only the views this article actually has: an empty tab is a + promise the article cannot keep. */} + {viewSwitch} +
-
- {/* A view with one section does not need a heading over it, or a - control to collapse the only thing there is. The tab already - names it — "Short" then a heading reading "In short" says the - same word twice and hides the content behind a chevron. */} - {soleSection ? ( -
- {soleSection.content} -
- ) : topSections.map(sec => renderSection(sec, 0))} -
- - {(article.references || []).length > 0 && ( -
-

References

- {/* Sources for the whole article, not markers in the prose: a - learner checking a claim wants the book and the page, and a - sentence peppered with superscripts is harder to read. */} -
    - {article.references.map((ref, index) => ( -
  1. - {ref.title} - {ref.author && — {ref.author}} - {(ref.pages || []).length > 0 && ( - · p. {ref.pages.join(', ')} - )} -
    2. - ))} -
-
- )} - - {children} + {prose} + {aside}
) } diff --git a/frontend/src/components/ArticleSaveButton.css b/frontend/src/components/ArticleSaveButton.css new file mode 100644 index 0000000..2ea6f9c --- /dev/null +++ b/frontend/src/components/ArticleSaveButton.css @@ -0,0 +1,45 @@ +/* Keeping a topic: the star in the article's toolbar, and the panel behind it. */ + +.asave { position: relative; display: inline-flex; } + +.asave-pop { + position: absolute; + top: calc(100% + 6px); + right: 0; + z-index: 40; + width: min(280px, calc(100vw - 32px)); + padding: 12px; + background: var(--card-bg); + border: 1px solid var(--border); + border-radius: 10px; + box-shadow: 0 12px 32px rgba(15, 23, 42, .16); + text-align: left; +} +.asave-title { margin: 0 0 2px; font-size: .84rem; font-weight: 700; } +.asave-note, .asave-empty { margin: 0 0 8px; font-size: .76rem; color: var(--text-muted); line-height: 1.45; } +.asave-empty { margin-bottom: 0; } + +.asave-find { + width: 100%; padding: 7px 9px; margin-bottom: 8px; + border: 1px solid var(--border); border-radius: 8px; + background: var(--input-bg); color: var(--text); font: inherit; font-size: .84rem; +} +.asave-find:focus { outline: 2px solid var(--primary); outline-offset: -1px; } + +.asave-new { + display: flex; align-items: center; justify-content: space-between; gap: 8px; + width: 100%; margin-bottom: 8px; padding: 8px 10px; + border: 1px dashed var(--border); border-radius: 8px; background: none; + font: inherit; font-size: .84rem; color: var(--text); cursor: pointer; +} +.asave-new:hover { border-color: var(--primary); color: var(--primary); } +.asave-new span:first-child { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } + +.asave-list { list-style: none; margin: 0; padding: 0; display: flex; flex-direction: column; gap: 3px; max-height: 210px; overflow-y: auto; } +.asave-list button { + width: 100%; text-align: left; padding: 7px 9px; + border: 0; border-radius: 7px; background: none; + font: inherit; font-size: .84rem; color: var(--text); cursor: pointer; +} +.asave-list button:hover { background: var(--option-hover, #eef4fb); } +.asave-list button.is-in { color: var(--text-muted); cursor: default; } diff --git a/frontend/src/components/ArticleSaveButton.jsx b/frontend/src/components/ArticleSaveButton.jsx new file mode 100644 index 0000000..c8cad8d --- /dev/null +++ b/frontend/src/components/ArticleSaveButton.jsx @@ -0,0 +1,155 @@ +import { useCallback, useEffect, useRef, useState } from 'react' +import api from '../api/client' +import './ArticleSaveButton.css' + +/** + * Keep this topic. + * + * Libraries hold questions — `user_collection_questions` has a question_id and + * nothing else — so saving a topic saves the questions written against it, + * which is what you would want it for: the reading and the practice on one + * subject, filed together, ready to sit later. Nothing here invents a second + * place to save things; it is the same endpoint the session player writes to + * when you file a question mid-attempt. + * + * The consequence is that the server cannot be asked "is this article saved?", + * only "is this question in that library?", and answering it properly would + * mean fetching every library's contents on every article — which also stamps + * each one as used and reorders the reader's own list. So the answer is kept + * on the device that gave it. It is honest about a save that happened; it will + * not know about one made on another machine. An `article_id` on the join + * table would settle it, and is the right fix when the API can change. + */ +const SAVED_KEY = 'pedshub.articleLibraries' + +const readSaved = () => { + try { return JSON.parse(localStorage.getItem(SAVED_KEY) || '{}') } catch { return {} } +} +const writeSaved = (next) => { + try { localStorage.setItem(SAVED_KEY, JSON.stringify(next)) } catch { /* the visit still knows */ } +} + +export default function ArticleSaveButton({ articleId, questionIds = [] }) { + const [open, setOpen] = useState(false) + const [libraries, setLibraries] = useState([]) + const [query, setQuery] = useState('') + const [busy, setBusy] = useState(false) + const [saved, setSaved] = useState(() => readSaved()[String(articleId)] || []) + const wrap = useRef(null) + + useEffect(() => { setSaved(readSaved()[String(articleId)] || []) }, [articleId]) + + useEffect(() => { + if (!open) return + api.get('/collections/').then(res => setLibraries(Array.isArray(res.data) ? res.data : [])) + .catch(() => setLibraries([])) + }, [open]) + + // A popover that outlives the click that dismissed it is a popover you have + // to close twice. + useEffect(() => { + if (!open) return undefined + const away = (event) => { if (!wrap.current?.contains(event.target)) setOpen(false) } + document.addEventListener('mousedown', away) + return () => document.removeEventListener('mousedown', away) + }, [open]) + + const record = useCallback((collectionId) => { + setSaved(prev => { + if (prev.includes(collectionId)) return prev + const next = [...prev, collectionId] + writeSaved({ ...readSaved(), [String(articleId)]: next }) + return next + }) + }, [articleId]) + + const saveInto = async (collectionId) => { + setBusy(true) + try { + // Sequential rather than parallel: a topic with forty questions firing + // forty writes at once is a burst the rate limiter reads as abuse. + for (const questionId of questionIds) { + await api.put(`/collections/${collectionId}/questions/${questionId}`) + } + record(collectionId) + setQuery('') + } catch { /* nothing is recorded, which is the honest signal */ } + finally { setBusy(false) } + } + + const createAndSave = async (title) => { + const name = title.trim() + if (!name) return + setBusy(true) + try { + const made = await api.post('/collections/', { title: name }) + setLibraries(list => [...list, made.data]) + setBusy(false) + await saveInto(made.data.id) + } catch { setBusy(false) } + } + + const needle = query.trim().toLowerCase() + const found = libraries.filter(row => row.title.toLowerCase().includes(needle)) + // Offered only when nothing you already have is called this — two libraries + // of the same name is a filing system nobody can use. + const canCreate = !!needle && !libraries.some(row => row.title.trim().toLowerCase() === needle) + const isSaved = saved.length > 0 + + return ( + + + {open && ( +
+

Save this topic

+ {questionIds.length === 0 ? ( + /* Said plainly rather than offering a control that would file + nothing: a library holds questions, and this topic has none yet. */ +

No questions are linked to this topic yet, so there is nothing to file. Link one and it can be saved with the reading.

+ ) : ( + <> +

Files its {questionIds.length} linked question{questionIds.length === 1 ? '' : 's'} so you can sit them later.

+ {/* One box for both. Searching what you have and naming what you + do not are the same act — you type the name of the library + you want and it either exists or it doesn't. */} + setQuery(event.target.value)} + onKeyDown={event => { + if (event.key === 'Enter' && canCreate) { event.preventDefault(); createAndSave(query) } + }} /> + {canCreate && ( + + )} + {found.length > 0 && ( +
    + {found.map(row => { + const inIt = saved.includes(row.id) + return ( +
  • + +
    • + ) + })} +
+ )} + {!found.length && !canCreate && ( +

{libraries.length ? 'No library by that name.' : 'Type a name to make your first library.'}

+ )} + + )} +
+ )} + + ) +} diff --git a/frontend/src/components/ArticleSplitPane.jsx b/frontend/src/components/ArticleSplitPane.jsx index 5c241ef..cce88f9 100644 --- a/frontend/src/components/ArticleSplitPane.jsx +++ b/frontend/src/components/ArticleSplitPane.jsx @@ -11,18 +11,28 @@ import './ArticleSplitPane.css' * The point of split view is that the article you came from stays put, so this * pane carries only the reading — no comments, no practice, no editing — and * the full article is one click away in the heading for anything more. + * + * It is one column of a grid the reading page already owns, not a second page + * nested inside the first. It used to render the whole reader, contents rail + * included, into a box half the width of the screen: two rails, two + * scrollbars, and prose that started further right with every cross-reference + * followed. A fixed column and a `bare` reader mean the article on the left + * reflows once, when the pane opens, and never again however deep you go. + * + * Depth is a trail rather than a stack of panes. Following a reference from + * inside the pane replaces what is in it and offers the way back, which is the + * thing that was actually wanted: read the aside, then return to the sentence + * that sent you. */ -export default function ArticleSplitPane({ slug, onClose }) { +export default function ArticleSplitPane({ slug, onClose, onBack, depth = 0 }) { const [article, setArticle] = useState(null) const [error, setError] = useState('') - const [activeSection, setActiveSection] = useState('') const pane = useRef(null) useEffect(() => { let live = true setArticle(null) setError('') - setActiveSection('') // The slug's id has already been fetched by the preview card that offered // the split, so opening the pane costs one request, not two. resolveArticleId(slug) @@ -42,6 +52,12 @@ export default function ArticleSplitPane({ slug, onClose }) { aria-label={article ? `Split view: ${article.title}` : 'Split view'} onKeyDown={event => { if (event.key === 'Escape') onClose() }}>
+ {depth > 0 && ( + + )} {article ? article.title : 'Opening…'} - -

- This question is shared — changes apply to all quizzes using it. -

- {error &&
{error}
} -
- -