diff --git a/src/components/announcements/AnnouncementModal.tsx b/src/components/announcements/AnnouncementModal.tsx index 1bcb372..5179778 100644 --- a/src/components/announcements/AnnouncementModal.tsx +++ b/src/components/announcements/AnnouncementModal.tsx @@ -1,8 +1,6 @@ import { useEffect, useRef } from 'react'; -import { TriangleAlert } from 'lucide-react'; import type { ClientAnnouncement } from './types.js'; -import { priorityTheme } from './theme.js'; -import { Markdown } from './Markdown.js'; +import { AnnouncementModalCard } from './AnnouncementModalCard.js'; export interface AnnouncementModalProps { announcement: ClientAnnouncement; @@ -15,25 +13,14 @@ const FOCUSABLE_SELECTOR = /** * The one surface in this library that interrupts a user rather than waiting - * to be noticed. Two flavours, chosen by priority, not by a prop the host - * could get wrong: - * - * - Editorial (major/normal): hero image, an Anton headline (22px+, the only - * size this library lets Anton run below its usual dashboard-heading size), - * the full markdown body, an accent CTA, "Later" as the quiet way out. - * - Alert (critical): no image — a screenshot of a release is exciting, a - * screenshot of an outage is not, and the image would cost a beat of - * reading time an incident notice can't spend — a warning glyph, a short - * body, an alert-red CTA, "Got it". - * - * Both trap focus while open and hand it back to whatever had it before the - * modal opened, on the theory that an announcement interrupts the page but - * must not lose the user's place on it. + * to be noticed. Everything visible lives in `AnnouncementModalCard`, which + * the admin composer's preview renders too — see that file for why the two are + * one component. This one owns only what makes the card a dialog: the + * backdrop, the dialog role, and a focus trap that hands focus back to + * whatever had it before the modal opened, on the theory that an announcement + * interrupts the page but must not lose the user's place on it. */ export function AnnouncementModal({ announcement, onDismiss, onCtaClick }: AnnouncementModalProps) { - const theme = priorityTheme(announcement.priority); - const isAlert = announcement.priority === 'critical'; - const dialogRef = useRef(null); const returnFocusTo = useRef(null); @@ -98,11 +85,6 @@ export function AnnouncementModal({ announcement, onDismiss, onCtaClick }: Annou // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - const handleCta = () => { - onCtaClick(announcement); - onDismiss(); - }; - return (
- {!isAlert && announcement.heroUrl && ( - // A non-empty alt is required here, not just nice-to-have: an empty - // alt gives an the "presentation" accessibility role instead - // of "img", which is also why an empty alt would fail this - // component's own hero-image test. - {announcement.title} - )} - -
- {isAlert ? ( -
- - - {theme.label} - -
- ) : ( - - {theme.label} - - )} - - {isAlert ? ( -

- {announcement.title} -

- ) : ( - // Anton reads cleanly at 16px+ only; this hero headline sits well - // above that floor, unlike the smaller UI labels elsewhere that - // stay on the body font instead. -

- {announcement.title} -

- )} - - {isAlert ? ( -

- {announcement.summary} -

- ) : ( -
- {announcement.body} -
- )} - -
- - {announcement.cta && ( - - )} -
-
+
); diff --git a/src/components/announcements/AnnouncementModalCard.tsx b/src/components/announcements/AnnouncementModalCard.tsx new file mode 100644 index 0000000..6179eac --- /dev/null +++ b/src/components/announcements/AnnouncementModalCard.tsx @@ -0,0 +1,134 @@ +import { TriangleAlert } from 'lucide-react'; +import type { ClientAnnouncement } from './types.js'; +import { priorityTheme } from './theme.js'; +import { Markdown } from './Markdown.js'; + +export interface AnnouncementModalCardProps { + announcement: ClientAnnouncement; + onDismiss: () => void; + onCtaClick: (announcement: ClientAnnouncement) => void; +} + +/** + * The announcement modal's card — everything inside the backdrop, and nothing + * about being a dialog. `AnnouncementModal` wraps this in the backdrop, the + * `role="dialog"` element and the focus trap; the admin composer's preview + * renders it bare. + * + * That split is the whole point of this file existing separately. The composer + * used to hand-build its own approximation of the modal, and the two drifted: + * the preview showed the full body for a critical announcement while the real + * modal showed only the popover summary, so an author proof-read text their + * readers never saw. A preview that re-implements the thing it previews is a + * lie waiting to happen — there is now exactly one component, and "the preview + * matches the modal" is true by construction rather than by review. + * + * Two flavours, chosen by priority, not by a prop the host could get wrong: + * + * - Editorial (major/normal): hero image, an Anton headline (22px+, the only + * size this library lets Anton run below its usual dashboard-heading size), + * the markdown body, an accent CTA, "Later" as the quiet way out. + * - Alert (critical): no image — a screenshot of a release is exciting, a + * screenshot of an outage is not, and the image would cost a beat of + * reading time an incident notice can't spend — a warning glyph, the same + * markdown body, an alert-red CTA, "Got it". + */ +export function AnnouncementModalCard({ announcement, onDismiss, onCtaClick }: AnnouncementModalCardProps) { + const theme = priorityTheme(announcement.priority); + const isAlert = announcement.priority === 'critical'; + + const handleCta = () => { + onCtaClick(announcement); + onDismiss(); + }; + + return ( +
+ {/* Only the read-me part scrolls. The actions below stay put, because a + modal that blocks the page must never push its own dismiss button + past the bottom of the viewport — a long critical body would + otherwise leave the reader with no way out but Escape. */} +
+ {!isAlert && announcement.heroUrl && ( + // A non-empty alt is required here, not just nice-to-have: an empty + // alt gives an the "presentation" accessibility role instead + // of "img", which is also why an empty alt would fail this + // component's own hero-image test. + {announcement.title} + )} + +
+ {isAlert ? ( +
+ + + {theme.label} + +
+ ) : ( + + {theme.label} + + )} + + {isAlert ? ( +

+ {announcement.title} +

+ ) : ( + // Anton reads cleanly at 16px+ only; this hero headline sits well + // above that floor, unlike the smaller UI labels elsewhere that + // stay on the body font instead. +

+ {announcement.title} +

+ )} + + {/* Both flavours render the body, never the summary: the summary is + the two-line teaser the bell popover shows, and a modal that + stopped at it dropped every instruction written below the first + paragraph — the part a critical notice exists to deliver. */} + {announcement.body} +
+
+ +
+ + {announcement.cta && ( + + )} +
+
+ ); +} diff --git a/src/components/announcements/__tests__/AnnouncementModal.test.tsx b/src/components/announcements/__tests__/AnnouncementModal.test.tsx index 5f43d0d..e92974a 100644 --- a/src/components/announcements/__tests__/AnnouncementModal.test.tsx +++ b/src/components/announcements/__tests__/AnnouncementModal.test.tsx @@ -59,6 +59,39 @@ describe('AnnouncementModal', () => { expect(container.textContent).toContain('after'); }); + it('renders the body for a critical announcement, not the one-line popover summary', () => { + // Regression: the alert flavour used to render `summary` — the two-line + // string the bell popover shows — so everything the author wrote below + // the first paragraph (the actual instructions a critical notice exists + // to deliver) never reached the reader, even though the composer's own + // preview showed the full body. + const critical = item({ + priority: 'critical', + summary: 'One line for the bell popover.', + body: 'Update to **0.14.3** and redeploy.\n\n## Building a bot?\n\nThere is more to do than the version.', + }); + render( {}} onCtaClick={() => {}} />); + + expect(screen.getByText('0.14.3').tagName).toBe('STRONG'); + expect(screen.getByRole('heading', { name: /building a bot/i })).toBeTruthy(); + expect(screen.getByText(/more to do than the version/)).toBeTruthy(); + // The summary is the popover's job; repeating it above a body derived + // from the same first paragraph would read as a stutter. + expect(screen.queryByText('One line for the bell popover.')).toBeNull(); + }); + + it('keeps the actions reachable when the body is longer than the viewport', () => { + // A blocking modal whose dismiss button has been pushed off-screen by a + // long body is a trap: the body scrolls, the header and actions do not. + const long = item({ priority: 'critical', body: Array.from({ length: 80 }, (_, i) => `Line ${i}.`).join('\n\n') }); + render( {}} onCtaClick={() => {}} />); + + const scroller = screen.getByTestId('announcement-modal-body'); + expect(scroller.className).toContain('overflow-y-auto'); + // The dismiss button must live outside the scrolling region. + expect(scroller.contains(screen.getByRole('button', { name: /got it/i }))).toBe(false); + }); + it('always shows "Got it" for a critical announcement, even when it has a cta', () => { const critical = item({ priority: 'critical', cta: { label: 'View status', url: '/status' } }); render( {}} onCtaClick={() => {}} />); diff --git a/src/components/announcements/index.ts b/src/components/announcements/index.ts index fec3d2e..f4a27f3 100644 --- a/src/components/announcements/index.ts +++ b/src/components/announcements/index.ts @@ -12,3 +12,8 @@ export { Markdown } from './Markdown.js'; export type { MarkdownProps } from './Markdown.js'; export { AnnouncementModal } from './AnnouncementModal.js'; export type { AnnouncementModalProps } from './AnnouncementModal.js'; +// Exported for the admin composer's preview, which renders the modal's card +// without the backdrop/dialog wrapper so what an author proof-reads is the +// component their readers get, not a second implementation of it. +export { AnnouncementModalCard } from './AnnouncementModalCard.js'; +export type { AnnouncementModalCardProps } from './AnnouncementModalCard.js';