Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ This project follows [Keep a Changelog](https://keepachangelog.com/) and
the others were invisible and unreachable. The marker now carries a small count, and tapping it
lists every note so you can open any of them. A verse with a single note is unchanged.
([#114](https://github.com/kbennett2000/songbird/issues/114))
- **The multiple-notes list is calmer and easier to read.** That list used to show each note's full
text as one long, run-together scroll. It's now a tidy stack of cards — one clear preview line per
note, with its tags, and obvious gaps between them (in dark mode too). Tap a note to open it in
full. ([#116](https://github.com/kbennett2000/songbird/issues/116))

## [1.6.0] — 2026-06-09

Expand Down
31 changes: 23 additions & 8 deletions frontend/src/components/AnnotationsPopover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,24 +76,39 @@ describe("AnnotationsPopover", () => {
expect(screen.getByText("grace")).toBeInTheDocument();
});

it("calls onOpen with the clicked annotation (reader case)", async () => {
it("calls onOpen with the clicked card's annotation (reader case)", async () => {
const onOpen = vi.fn();
renderPopover([ann(1), ann(2)], { onOpen });
const openButtons = screen.getAllByRole("button", { name: "Open →" });
await userEvent.click(openButtons[1]!);
// Newest-first: row 0 is Note 1 (newer created_at default tie → stable), so assert by call arg id.
// The whole card is the button; its accessible name carries the note's preview.
await userEvent.click(screen.getByRole("button", { name: "Open note: Note 2" }));
expect(onOpen).toHaveBeenCalledTimes(1);
expect(onOpen.mock.calls[0]![0]).toMatchObject({ id: expect.any(Number) });
expect(onOpen.mock.calls[0]![0]).toMatchObject({ id: 2 });
});

it("falls back to reader deep-links when onOpen is absent (compare case)", () => {
it("makes each card a reader deep-link when onOpen is absent (compare case)", () => {
renderPopover([ann(1), ann(2)]);
expect(screen.queryByRole("button", { name: "Open →" })).not.toBeInTheDocument();
const links = screen.getAllByRole("link", { name: /Open in reader/ });
expect(screen.queryByRole("button", { name: /Open note/ })).not.toBeInTheDocument();
const links = screen.getAllByRole("link", { name: /Open note in reader/ });
expect(links).toHaveLength(2);
expect(links[0]).toHaveAttribute("href", "/read?book=JOS&chapter=1&verse=1");
});

it("shows a stripped, truncated preview — never a wall of raw Markdown (#116)", () => {
const long = `# Big Heading\n\n- bullet\n\n${"x".repeat(200)}`;
renderPopover([ann(1, { note_markdown: long })]);
const preview = screen.getByText(/^Big Heading/);
expect(preview.textContent).not.toContain("#");
expect(preview.textContent).not.toContain("- bullet"); // the "-" Markdown noise is stripped
expect(preview.textContent!.endsWith("…")).toBe(true);
expect(preview.textContent!.length).toBeLessThanOrEqual(121); // notePreview caps at 120 + "…"
expect(screen.queryByText(long)).not.toBeInTheDocument(); // the raw note is NOT dumped verbatim
});

it("labels an empty note rather than rendering a blank card", () => {
renderPopover([ann(1, { note_markdown: " " })], { onOpen: vi.fn() });
expect(screen.getByRole("button", { name: "Open note: (empty note)" })).toBeInTheDocument();
});

it("closes on Escape (shared Popover shell)", async () => {
const { onClose } = renderPopover([ann(1), ann(2)]);
await userEvent.keyboard("{Escape}");
Expand Down
118 changes: 69 additions & 49 deletions frontend/src/components/AnnotationsPopover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { useMemo } from "react";
import { Link } from "react-router-dom";

import { Popover } from "@/components/Popover";
import { readerLink } from "@/lib/notes";
import { notePreview, readerLink } from "@/lib/notes";
import type { ReadAnnotation } from "@/schemas";

interface AnnotationsPopoverProps {
Expand All @@ -12,8 +12,8 @@ interface AnnotationsPopoverProps {
anchor: HTMLElement;
onClose: () => void;
/**
* Reader case: open this note for editing. When provided, each row gets an "Open" button.
* When omitted (compare case), each row instead deep-links back to the reader (read-only).
* Reader case: open this note for editing. When provided, each card is a button that opens it.
* When omitted (compare case), each card is instead a deep-link back to the reader (read-only).
*/
onOpen?: (annotation: ReadAnnotation) => void;
}
Expand All @@ -23,11 +23,54 @@ function byNewest(a: ReadAnnotation, b: ReadAnnotation): number {
return a.created_at < b.created_at ? 1 : a.created_at > b.created_at ? -1 : 0;
}

const CARD_CLASS =
"block w-full text-left rounded border border-gray-200 dark:border-gray-700 bg-white dark:bg-gray-800 p-3 hover:bg-gray-50 dark:hover:bg-gray-700";

/** The body shared by both card variants: out-of-scope line, one-line preview + chevron, tags. */
function CardBody({ annotation }: { annotation: ReadAnnotation }): JSX.Element {
const preview = notePreview(annotation.note_markdown);
return (
<>
{!annotation.in_scope && annotation.scope_translations.length > 0 && (
<p className="mb-1 text-xs italic text-gray-500 dark:text-gray-400">
Written for {annotation.scope_translations.join(", ")}
</p>
)}
<div className="flex items-center gap-2">
<span
className={`min-w-0 flex-1 truncate text-sm ${
preview ? "text-gray-700 dark:text-gray-200" : "italic text-gray-400 dark:text-gray-500"
}`}
>
{preview || "(empty note)"}
</span>
<span aria-hidden className="shrink-0 text-gray-400">
</span>
</div>
{annotation.tags.length > 0 && (
<ul className="mt-2 flex flex-wrap gap-1">
{annotation.tags.map((tag) => (
<li
key={tag}
className="rounded-full bg-gray-100 dark:bg-gray-700 px-2 py-0.5 text-xs font-medium text-gray-600 dark:text-gray-300"
>
{tag}
</li>
))}
</ul>
)}
</>
);
}

/**
* A floating popover listing EVERY annotation on a verse — the multi-note case, so none is hidden
* behind `[0]` (#114). One tap shows the whole stack for the verse; each row reuses the single
* {@link AnnotationPopover} body (verbatim Markdown — invariant 6, never editor-native JSON — its
* out-of-scope line and tags). Shares the {@link Popover} shell, so a tall list scrolls inside.
* behind `[0]` (#114). Each note is a calm, bounded card showing a one-line plain-text preview
* ({@link notePreview} strips the Markdown — never a scary wall of raw `#`/`*`), tappable to open
* the full note: the editor in the reader, or a reader deep-link in compare (#116). Distinct
* bordered cards make the breaks between notes obvious in light and dark alike. Shares the
* {@link Popover} shell, so a long list scrolls inside the popover.
*/
export function AnnotationsPopover({
annotations,
Expand All @@ -38,7 +81,7 @@ export function AnnotationsPopover({
const sorted = useMemo(() => [...annotations].sort(byNewest), [annotations]);
return (
<Popover anchor={anchor} onClose={onClose} ariaLabel={`${annotations.length} notes on this verse`}>
<div className="mb-1 flex items-start justify-between gap-2">
<div className="mb-2 flex items-start justify-between gap-2">
<span className="text-xs font-semibold uppercase tracking-wide text-amber-700">
Notes · {annotations.length}
</span>
Expand All @@ -52,50 +95,27 @@ export function AnnotationsPopover({
</button>
</div>

<ul className="flex flex-col">
<ul className="flex flex-col gap-3">
{sorted.map((annotation) => (
<li
key={annotation.id}
className="border-t border-gray-100 py-2 first:border-0 first:pt-0"
>
{!annotation.in_scope && annotation.scope_translations.length > 0 && (
<p className="mb-1 text-xs italic text-gray-500 dark:text-gray-400">
Written for {annotation.scope_translations.join(", ")}
</p>
)}
<p className="whitespace-pre-wrap break-words text-gray-800 dark:text-gray-100">
{annotation.note_markdown}
</p>
{annotation.tags.length > 0 && (
<ul className="mt-2 flex flex-wrap gap-1">
{annotation.tags.map((tag) => (
<li
key={tag}
className="rounded-full bg-gray-100 dark:bg-gray-700 px-2 py-0.5 text-xs font-medium text-gray-600 dark:text-gray-300"
>
{tag}
</li>
))}
</ul>
<li key={annotation.id}>
{onOpen ? (
<button
type="button"
className={CARD_CLASS}
onClick={() => onOpen(annotation)}
aria-label={`Open note: ${notePreview(annotation.note_markdown) || "(empty note)"}`}
>
<CardBody annotation={annotation} />
</button>
) : (
<Link
to={readerLink(annotation)}
className={CARD_CLASS}
aria-label={`Open note in reader: ${notePreview(annotation.note_markdown) || "(empty note)"}`}
>
<CardBody annotation={annotation} />
</Link>
)}
<div className="mt-2">
{onOpen ? (
<button
type="button"
className="text-sm font-medium text-blue-700 dark:text-blue-400 hover:underline"
onClick={() => onOpen(annotation)}
>
Open →
</button>
) : (
<Link
to={readerLink(annotation)}
className="text-sm font-medium text-blue-700 dark:text-blue-400 hover:underline"
>
Open in reader →
</Link>
)}
</div>
</li>
))}
</ul>
Expand Down
9 changes: 4 additions & 5 deletions frontend/src/routes/ReaderView.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import { render, screen, waitFor, within } from "@testing-library/react";
import { render, screen, waitFor } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { http, HttpResponse } from "msw";
import { MemoryRouter } from "react-router-dom";
Expand Down Expand Up @@ -554,10 +554,9 @@ describe("ReaderView", () => {
expect(screen.getByText("first note")).toBeInTheDocument();
expect(screen.getByText("second note")).toBeInTheDocument();

// "Open" on the second row hands THAT note to the editor (pre-filled with its Markdown).
// Both notes share a created_at, so locate the row by its text rather than by order.
const secondRow = screen.getByText("second note").closest("li")!;
await user.click(within(secondRow).getByRole("button", { name: "Open →" }));
// Tapping a card hands THAT note to the editor (pre-filled with its Markdown). The whole
// card is the button; its accessible name carries the note's preview.
await user.click(screen.getByRole("button", { name: "Open note: second note" }));
expect(await screen.findByTestId("initial-markdown")).toHaveTextContent("second note");
});

Expand Down
Loading