Skip to content

fix(announcements): show the body in a critical modal, not the popover summary #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
KruGoL merged 1 commit into from
Aug 10, 2026
Merged

fix(announcements): show the body in a critical modal, not the popover summary #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
116 changes: 9 additions & 107 deletions src/components/announcements/AnnouncementModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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<HTMLDivElement>(null);
const returnFocusTo = useRef<HTMLElement | null>(null);

Expand Down Expand Up @@ -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 (
<div
className="fixed inset-0 flex items-center justify-center z-50 p-4"
Expand All @@ -114,89 +96,9 @@ export function AnnouncementModal({ announcement, onDismiss, onCtaClick }: Annou
aria-modal="true"
aria-label={announcement.title}
tabIndex={-1}
className="w-full max-w-lg rounded-xl overflow-hidden outline-none"
style={{ background: 'var(--bg-elevated)', border: '1px solid var(--border)', boxShadow: 'var(--shadow-lg)' }}
className="w-full flex justify-center outline-none"
>
{!isAlert && announcement.heroUrl && (
// A non-empty alt is required here, not just nice-to-have: an empty
// alt gives an <img> the "presentation" accessibility role instead
// of "img", which is also why an empty alt would fail this
// component's own hero-image test.
<img src={announcement.heroUrl} alt={announcement.title} className="w-full h-40 object-cover" />
)}

<div className="p-6">
{isAlert ? (
<div className="flex items-center gap-2 mb-3">
<TriangleAlert className="w-5 h-5 shrink-0" style={{ color: theme.accent }} />
<span className="text-xs font-semibold uppercase tracking-wide" style={{ color: theme.accent }}>
{theme.label}
</span>
</div>
) : (
<span
className="inline-block text-xs font-semibold uppercase tracking-wide mb-2"
style={{ color: theme.accent }}
>
{theme.label}
</span>
)}

{isAlert ? (
<h2 className="text-base font-semibold mb-3" style={{ color: 'var(--text-primary)' }}>
{announcement.title}
</h2>
) : (
// 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.
<h2
className="mb-4"
style={{ fontFamily: 'var(--font-display)', fontSize: '1.5rem', letterSpacing: '0.02em', color: 'var(--text-primary)' }}
>
{announcement.title}
</h2>
)}

{isAlert ? (
<p className="text-sm mb-6" style={{ color: 'var(--text-secondary)' }}>
{announcement.summary}
</p>
) : (
<div className="mb-6">
<Markdown>{announcement.body}</Markdown>
</div>
)}

<div className="flex items-center justify-end gap-3">
<button
type="button"
onClick={onDismiss}
className="text-sm font-medium px-4 py-2 rounded-lg transition-colors hover:bg-white/5"
style={{ color: 'var(--text-secondary)' }}
>
{/* Flavour decides first, CTA presence second — not the other
way around. Alert is "do something now": the acknowledgement
is always "Got it", whether or not there's also a CTA to act
on (a critical announcement with a CTA must not say "Later"
to something urgent). Editorial is "look what we shipped":
"Later" only makes sense when there's something to defer,
i.e. a CTA — with none, this is the sole action and reads as
"Got it" too. */}
{isAlert ? 'Got it' : (announcement.cta ? 'Later' : 'Got it')}
</button>
{announcement.cta && (
<button
type="button"
onClick={handleCta}
className="text-sm font-semibold px-4 py-2 rounded-lg text-white transition-opacity hover:opacity-90"
style={{ background: isAlert ? theme.accent : 'var(--accent)' }}
>
{announcement.cta.label}
</button>
)}
</div>
</div>
<AnnouncementModalCard announcement={announcement} onDismiss={onDismiss} onCtaClick={onCtaClick} />
</div>
</div>
);
Expand Down
134 changes: 134 additions & 0 deletions src/components/announcements/AnnouncementModalCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 (
<div
className="w-full max-w-lg rounded-xl overflow-hidden flex flex-col max-h-[85vh]"
style={{ background: 'var(--bg-elevated)', border: '1px solid var(--border)', boxShadow: 'var(--shadow-lg)' }}
>
{/* 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. */}
<div data-testid="announcement-modal-body" className="overflow-y-auto">
{!isAlert && announcement.heroUrl && (
// A non-empty alt is required here, not just nice-to-have: an empty
// alt gives an <img> the "presentation" accessibility role instead
// of "img", which is also why an empty alt would fail this
// component's own hero-image test.
<img src={announcement.heroUrl} alt={announcement.title} className="w-full h-40 object-cover" />
)}

<div className="p-6 pb-4">
{isAlert ? (
<div className="flex items-center gap-2 mb-3">
<TriangleAlert className="w-5 h-5 shrink-0" style={{ color: theme.accent }} />
<span className="text-xs font-semibold uppercase tracking-wide" style={{ color: theme.accent }}>
{theme.label}
</span>
</div>
) : (
<span
className="inline-block text-xs font-semibold uppercase tracking-wide mb-2"
style={{ color: theme.accent }}
>
{theme.label}
</span>
)}

{isAlert ? (
<h2 className="text-base font-semibold mb-3" style={{ color: 'var(--text-primary)' }}>
{announcement.title}
</h2>
) : (
// 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.
<h2
className="mb-4"
style={{ fontFamily: 'var(--font-display)', fontSize: '1.5rem', letterSpacing: '0.02em', color: 'var(--text-primary)' }}
>
{announcement.title}
</h2>
)}

{/* 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. */}
<Markdown>{announcement.body}</Markdown>
</div>
</div>

<div className="flex items-center justify-end gap-3 px-6 pb-6 pt-2 shrink-0">
<button
type="button"
onClick={onDismiss}
className="text-sm font-medium px-4 py-2 rounded-lg transition-colors hover:bg-white/5"
style={{ color: 'var(--text-secondary)' }}
>
{/* Flavour decides first, CTA presence second — not the other
way around. Alert is "do something now": the acknowledgement
is always "Got it", whether or not there's also a CTA to act
on (a critical announcement with a CTA must not say "Later"
to something urgent). Editorial is "look what we shipped":
"Later" only makes sense when there's something to defer,
i.e. a CTA — with none, this is the sole action and reads as
"Got it" too. */}
{isAlert ? 'Got it' : (announcement.cta ? 'Later' : 'Got it')}
</button>
{announcement.cta && (
<button
type="button"
onClick={handleCta}
className="text-sm font-semibold px-4 py-2 rounded-lg text-white transition-opacity hover:opacity-90"
style={{ background: isAlert ? theme.accent : 'var(--accent)' }}
>
{announcement.cta.label}
</button>
)}
</div>
</div>
);
}
33 changes: 33 additions & 0 deletions src/components/announcements/__tests__/AnnouncementModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(<AnnouncementModal announcement={critical} onDismiss={() => {}} 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(<AnnouncementModal announcement={long} onDismiss={() => {}} 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(<AnnouncementModal announcement={critical} onDismiss={() => {}} onCtaClick={() => {}} />);
Expand Down
5 changes: 5 additions & 0 deletions src/components/announcements/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Loading