diff --git a/packages/shared/src/components/notifications/NotificationItem.spec.tsx b/packages/shared/src/components/notifications/NotificationItem.spec.tsx index 92383ac3425..e52b324b59d 100644 --- a/packages/shared/src/components/notifications/NotificationItem.spec.tsx +++ b/packages/shared/src/components/notifications/NotificationItem.spec.tsx @@ -134,6 +134,93 @@ describe('notification attachment', () => { }); }); +describe('content-arrival rows', () => { + const hoursAgo = (h: number) => new Date(Date.now() - h * 3_600_000); + const sourcePost: NotificationItemProps = { + icon: NotificationIconType.Bell, + type: NotificationType.SourcePostAdded, + title: '

New post in The New Stack

', + referenceId: 'source-post', + targetUrl: 'post url', + createdAt: hoursAgo(3), + avatars: [sampleNotificationAvatars[0]], + attachments: [ + { + title: "Anthropic's Claude now has a browser of its own", + image: 'cover', + type: NotificationAttachmentType.Post, + }, + ], + }; + + it('should lead with the post headline, not the announcing sentence', async () => { + renderComponent(); + const headline = await screen.findByText( + "Anthropic's Claude now has a browser of its own", + ); + expect(headline).toHaveClass('font-bold'); + expect(screen.queryAllByText(/New post in/)).toHaveLength(0); + await screen.findByText('source'); + }); + + it('should credit the person and the squad when both are present', async () => { + renderComponent( + , + ); + await screen.findByText('user in source'); + }); + + it('should keep the actor-first layout when there is no post to promote', async () => { + renderComponent(); + // The source sits in a , so the sentence spans several text nodes. + const matches = await screen.findAllByText(/New post in/); + expect(matches.length).toBeGreaterThan(0); + }); +}); + +describe('notification timestamp', () => { + const hoursAgo = (h: number) => new Date(Date.now() - h * 3_600_000); + + it('should follow the last grey line rather than the title', async () => { + renderComponent( + , + ); + const time = await screen.findByText('5h'); + expect(time.closest('div')).toHaveTextContent(/Sample attachment\s*·\s*5h/); + }); + + it('should never sit inside a clamped line, so a long title cannot hide it', async () => { + renderComponent( + , + ); + const time = await screen.findByText('5h'); + expect(time.closest('.line-clamp-1, .line-clamp-2')).toBeNull(); + expect(time.closest('.truncate')).toBeNull(); + }); + + it('should take a line of its own when the row has no grey text', async () => { + renderComponent( + , + ); + const time = await screen.findByText('9h'); + expect(time.closest('div')).toHaveTextContent('9h'); + expect(time.closest('div')).not.toHaveTextContent('·'); + }); +}); + describe('notification avatars', () => { it('should display the avatar of the source', async () => { const [source] = sampleNotificationAvatars; diff --git a/packages/shared/src/components/notifications/NotificationItem.tsx b/packages/shared/src/components/notifications/NotificationItem.tsx index f9a5adab6ad..cc70a88de81 100644 --- a/packages/shared/src/components/notifications/NotificationItem.tsx +++ b/packages/shared/src/components/notifications/NotificationItem.tsx @@ -8,8 +8,12 @@ import { useObjectPurify } from '../../hooks/useDomPurify'; import NotificationItemAvatar from './NotificationItemAvatar'; import { NotificationItemLead } from './NotificationItemLead'; import { NotificationCategoryBadge } from './NotificationCategoryBadge'; -import { getNotificationLeadAvatar } from './leadAvatar'; import { + getNotificationAttribution, + getNotificationLeadAvatar, +} from './leadAvatar'; +import { + contentArrivalNotificationTypes, getNotificationCategory, NotificationFilterCategory, notificationMutingCopy, @@ -295,6 +299,42 @@ function NotificationItem(props: NotificationItemProps): ReactElement | null { attachmentTitleNorm !== titleNorm && attachmentTitleNorm !== descriptionNorm; + const isContentArrival = + contentArrivalNotificationTypes.has(type) && !!attachmentTitle; + const attribution = isContentArrival + ? getNotificationAttribution(filteredAvatars) + : undefined; + const showSubtitle = isContentArrival + ? showDescription + : showDescription || showAttachmentTitle; + const showAttachmentLine = + !isContentArrival && showDescription && showAttachmentTitle; + + let timeAnchor: 'attribution' | 'attachment' | 'subtitle' | null = null; + if (attribution) { + timeAnchor = 'attribution'; + } else if (showAttachmentLine) { + timeAnchor = 'attachment'; + } else if (showSubtitle) { + timeAnchor = 'subtitle'; + } + + const timeNode = timeText ? ( + + + + ) : null; + const timeTail = timeNode && ( + <> + · + {timeNode} + + ); + // A global `* { flex-shrink: 0 }` means truncating text must opt back in with + // `shrink`, or it keeps max-content width and overflows the row. + const greyLine = + 'flex items-baseline gap-1.5 text-text-tertiary typo-footnote'; + return (
- {/* Headline (actor name bold, the rest regular for a scannable - hierarchy) with the relative time flowing inline right after it — so a - row reads "what happened" first and the time is a quiet suffix, not a - right-aligned column. Then the comment, then the post's title. */}
-
- - {timeText && ( - - - - )} -
- {(showDescription || showAttachmentTitle) && ( -
- {showDescription ? ( - + {isContentArrival ? ( +
+ {attachmentTitle} +
+ ) : ( +
+ +
+ )} + {showSubtitle && ( + <> + {timeAnchor === 'subtitle' ? ( + // `multi-truncate` is `display: -webkit-box`, which blockifies + // as a flex item and loses both its clamp and its width — hence + // the separate single-line branch whenever the time rides here. +
+ + {showDescription ? ( + + ) : ( + showAttachmentTitle && attachmentTitle + )} + + + {timeTail} + +
) : ( - showAttachmentTitle && {attachmentTitle} +
+ {showDescription ? ( + + ) : ( + showAttachmentTitle && {attachmentTitle} + )} +
)} -
+ )} {/* When there's both a comment and a post, name the post on its own line so it's clear which article it's about. */} - {showDescription && showAttachmentTitle && ( -
- {attachmentTitle} + {showAttachmentLine && ( +
+ {attachmentTitle} + {timeAnchor === 'attachment' && timeTail} +
+ )} + {attribution && ( +
+ {attribution} + {timeAnchor === 'attribution' && timeTail}
)} + {!timeAnchor && timeNode &&
{timeNode}
} {type === NotificationType.UserFollow && ( diff --git a/packages/shared/src/components/notifications/leadAvatar.ts b/packages/shared/src/components/notifications/leadAvatar.ts index 696d859f26f..9f96e3174fa 100644 --- a/packages/shared/src/components/notifications/leadAvatar.ts +++ b/packages/shared/src/components/notifications/leadAvatar.ts @@ -17,3 +17,18 @@ export const getNotificationLeadAvatar = ( ): NotificationAvatar | undefined => avatars.find((avatar) => avatar.type === NotificationAvatarType.User) ?? avatars[0]; + +export const getNotificationAttribution = ( + avatars: NotificationAvatar[] = [], +): string | undefined => { + const source = avatars.find( + (avatar) => avatar.type === NotificationAvatarType.Source, + ); + const user = avatars.find( + (avatar) => avatar.type === NotificationAvatarType.User, + ); + if (source?.name && user?.name) { + return `${user.name} in ${source.name}`; + } + return source?.name ?? user?.name; +}; diff --git a/packages/shared/src/components/notifications/utils.ts b/packages/shared/src/components/notifications/utils.ts index 1066fcd54ed..9bb01469640 100644 --- a/packages/shared/src/components/notifications/utils.ts +++ b/packages/shared/src/components/notifications/utils.ts @@ -223,6 +223,12 @@ export const notificationTypeTheme: Partial> = [NotificationType.UserFollow]: 'text-brand-default', }; +export const contentArrivalNotificationTypes = new Set([ + NotificationType.SourcePostAdded, + NotificationType.SquadPostAdded, + NotificationType.UserPostAdded, +]); + export const notificationTypeNotClickable: Partial< Record > = { diff --git a/packages/storybook/stories/components/NotificationItem.stories.tsx b/packages/storybook/stories/components/NotificationItem.stories.tsx index 534142e2440..2941fbd2fca 100644 --- a/packages/storybook/stories/components/NotificationItem.stories.tsx +++ b/packages/storybook/stories/components/NotificationItem.stories.tsx @@ -43,6 +43,14 @@ const meta: Meta = { title: 'Components/Notifications/List item — all types', component: NotificationItem, tags: ['autodocs'], + parameters: { + docs: { + description: { + component: + 'Every notification type rendered by the real component. See "Row anatomy" for the layout rules these rows follow — which line leads, and where the timestamp lands.', + }, + }, + }, decorators: [ (Story) => ( @@ -253,7 +261,10 @@ const allDefs: Array> = [ title: 'New post in Agentic Digest', avatars: [sourceAvatar('agentic', 'Agentic Digest')], attachments: [ - postAttachment('p1', 'MAI-Code-1-Flash beats Claude Haiku 4.5 on SWE-Bench'), + postAttachment( + 'p1', + 'MAI-Code-1-Flash beats Claude Haiku 4.5 on SWE-Bench', + ), ], createdAt: hoursAgo(23), }, @@ -528,3 +539,140 @@ export const MobileViewport: Story = { }, render: () => framed(), }; + +// ---- Row anatomy: the layout rules, with a live row for each -------------- + +interface AnatomyCase { + label: string; + rule: string; + notification: NotificationItemProps; +} + +const anatomyCases: AnatomyCase[] = [ + { + label: 'Content arrival — a source posted', + rule: 'The post title leads in bold typo-callout. The announcing sentence ("New post in …") is dropped and rebuilt from the avatars as a quiet attribution, with the time trailing it.', + notification: { + type: NotificationType.SourcePostAdded, + icon: NotificationIconType.Bell, + title: 'New post in The New Stack', + avatars: [sourceAvatar('tns', 'The New Stack')], + attachments: [ + postAttachment( + 'tns', + "Anthropic's Claude now has a browser of its own", + ), + ], + createdAt: hoursAgo(3), + referenceId: 'anatomy-source', + targetUrl: '/post/1', + onClick: fn(), + }, + }, + { + label: 'Content arrival — a person posted into a squad', + rule: 'Same shape, but the attribution names both: " in ". Built from the avatars, never from the server sentence.', + notification: { + type: NotificationType.SquadPostAdded, + icon: NotificationIconType.Bell, + title: 'GeekLuffy posted in AI', + avatars: [sourceAvatar('ai', 'AI'), userAvatar('luffy', 'Luffy')], + attachments: [ + postAttachment('sq', 'Fine-tuning on a budget: what actually moved'), + ], + createdAt: hoursAgo(7), + referenceId: 'anatomy-squad', + targetUrl: '/post/2', + onClick: fn(), + }, + }, + { + label: 'Social — the person is the payload', + rule: 'Actor-first, unchanged. Grey lines follow: the comment, then the post it happened on. The time trails the LAST grey line — here the post title.', + notification: { + type: NotificationType.ArticleNewComment, + icon: NotificationIconType.Comment, + title: 'Nimrod Kramer commented on your post', + description: 'Great write-up — the part about caching really helped.', + avatars: [userAvatar('nimrod', 'Nimrod')], + attachments: [postAttachment('c1', 'Scaling our cache layer')], + createdAt: hoursAgo(5), + referenceId: 'anatomy-comment', + targetUrl: '/post/3', + onClick: fn(), + }, + }, + { + label: 'Social — no grey text at all', + rule: 'A bare row has nothing for the time to follow, so it takes a line of its own. This is the only case where the timestamp stands alone.', + notification: { + type: NotificationType.UserFollow, + icon: NotificationIconType.User, + title: 'Tobias Wolf started following you', + avatars: [userAvatar('tobias', 'Tobias')], + createdAt: hoursAgo(9), + referenceId: 'anatomy-follow', + targetUrl: '/tobias', + onClick: fn(), + }, + }, +]; + +export const RowAnatomy: Story = { + name: 'Row anatomy (layout rules)', + parameters: { layout: 'fullscreen' }, + render: () => ( +
+
+

Row anatomy

+

+ A notification inbox carries two genres of row. Content arrival{' '} + (a post showed up) makes the article headline the payload; the + sentence announcing it is boilerplate that repeats on every row.{' '} + Social (someone acted) makes the person the payload. The row + leads with whichever one it is. +

+

+ The timestamp never rides the leading line. It follows the row's + last grey line, so it reads as part of the metadata trail instead of + competing with the thing you are trying to read. +

+
+ {anatomyCases.map((item) => ( +
+

+ {item.label} +

+

+ {item.rule} +

+
+ +
+
+ ))} +
+

+ Two traps when editing this row +

+
    +
  • + The time must stay a flex sibling of the truncating text, + never inside its clamp. Inline-inside-the-clamp is how a long + headline hides the timestamp entirely at mobile width. +
  • +
  • + multi-truncate cannot be used on a line the time rides: + it is display: -webkit-box, which blockifies as a flex + item and loses both its clamp and its width. And a global{' '} + + * {'{'} flex-shrink: 0 {'}'} + {' '} + means the text has to opt back into shrinking, or it overflows the + row instead of ellipsing. +
  • +
+
+
+ ), +}; diff --git a/packages/storybook/stories/components/notifications/FullPage.stories.tsx b/packages/storybook/stories/components/notifications/FullPage.stories.tsx index d111b00b559..14efd82256b 100644 --- a/packages/storybook/stories/components/notifications/FullPage.stories.tsx +++ b/packages/storybook/stories/components/notifications/FullPage.stories.tsx @@ -21,8 +21,8 @@ import { groupByTime, sampleNotifications } from './_mock'; // Faithful, provider-light reconstruction of the /notifications page // (packages/webapp/components/notifications/NotificationsFeed.tsx) using the // real shared NotificationItem rows + the same filter bar primitive and time -// grouping. The live page is auth-gated and needs backend data, so this is the -// canvas to iterate on the page's readability. +// grouping. The live page is auth-gated and needs backend data, so this is +// where the shipped row layout can be seen in a full list. const meta: Meta = { title: 'Components/Notifications/Full page', @@ -31,7 +31,7 @@ const meta: Meta = { docs: { description: { component: - 'Reconstruction of the /notifications page — header, type filters, time-grouped feed of NotificationItem rows. Use the filter tabs to scope by category. The real page lives in NotificationsFeed.tsx and is auth-gated, so this is where to review page-level readability.', + 'Reconstruction of the /notifications page — header, type filters, time-grouped feed of NotificationItem rows. Use the filter tabs to scope by category. The real page lives in NotificationsFeed.tsx and is auth-gated. Post arrivals lead with the headline here; see "Row anatomy" for the rules.', }, }, }, diff --git a/packages/storybook/stories/components/notifications/InAppPopup.stories.tsx b/packages/storybook/stories/components/notifications/InAppPopup.stories.tsx index 32288f1a257..be2733ec0a5 100644 --- a/packages/storybook/stories/components/notifications/InAppPopup.stories.tsx +++ b/packages/storybook/stories/components/notifications/InAppPopup.stories.tsx @@ -16,6 +16,13 @@ import { userAvatar, sourceAvatar } from './_mock'; // InAppNotification.tsx wraps InAppNotificationItem in a pepper-subtler card // with a close button. Reproduced statically here so its 3-line clamp and // icon+avatar lockup can be reviewed. +// +// KNOWN GAP: this component renders the title and nothing else — no post +// headline, no timestamp. The feed row now leads a post arrival with the +// article headline, so the same notification reads "New post in " here +// and as the headline there. The avatar lockup is shared (NotificationItemLead) +// but the text layout is not. Worth closing deliberately rather than by +// accident. const meta: Meta = { title: 'Components/Notifications/In-app popup', diff --git a/packages/storybook/stories/components/notifications/Overview.stories.tsx b/packages/storybook/stories/components/notifications/Overview.stories.tsx index a4811b47e38..c3418fea006 100644 --- a/packages/storybook/stories/components/notifications/Overview.stories.tsx +++ b/packages/storybook/stories/components/notifications/Overview.stories.tsx @@ -2,9 +2,8 @@ import React from 'react'; import type { Meta, StoryObj } from '@storybook/react-vite'; import ExtensionProviders from '../../extension/_providers'; -// Landing page for the notifications readability audit. Collects every -// notification surface in one folder and calls out the concrete levers to pull -// on each one, so improving readability is a guided pass rather than a hunt. +// Landing page for the notification surfaces: the layout rules the feed row +// now follows, then a map of every surface and what to check on each. const meta: Meta = { title: 'Components/Notifications/Overview', @@ -13,7 +12,7 @@ const meta: Meta = { docs: { description: { component: - 'Start here. A map of every notification surface, where it lives in the codebase, and the specific readability levers to review on each.', + 'Start here. The layout rules a notification row follows, a map of every notification surface and where it lives, and the known gaps between them.', }, }, }, @@ -65,8 +64,8 @@ const surfaces: SurfaceRow[] = [ surface: 'A single feed row (NotificationItem)', source: 'shared/components/notifications/NotificationItem.tsx', levers: [ - 'Title weight/size (bold typo-callout) vs. 2-line clamp', - 'Meta line: time + description at typo-footnote text-tertiary — is it readable?', + 'Which line leads — headline on a post arrival, actor sentence otherwise (see "Row anatomy")', + 'Where the timestamp lands: it follows the last grey line, or stands alone when there is none', 'When description AND post title both show — is the hierarchy clear?', 'Unread state (bg-surface-float) — is it distinct enough?', 'Avatar + corner badge + attachment alignment down the column', @@ -88,6 +87,7 @@ const surfaces: SurfaceRow[] = [ surface: 'Real-time push-style popup', source: 'shared/components/notifications/InAppNotificationItem.tsx', levers: [ + 'KNOWN GAP: renders the title only — no headline, no post, no time. A post arrival still reads "New post in " here while the feed row leads with the headline.', 'Title contrast on the accent-pepper-subtler card', '3-line clamp — where do long titles get cut?', 'Icon + avatar lockup spacing', @@ -113,14 +113,43 @@ const Pill = ({ children }: { children: React.ReactNode }) => ( const NotificationsOverview = (): React.ReactElement => (
-

Notifications — readability audit

+

Notifications

- Every notification surface, collected in one folder. The feedback is that - notifications aren't readable enough — so each story below isolates a - surface and lists the concrete levers to tune. Toggle the Storybook theme - (light / dark) on each to check contrast both ways. Work top to bottom. + Every notification surface, collected in one folder. The row rules are + below; each story then isolates a surface and lists what to check on it. + Toggle the Storybook theme (light / dark) on each to check contrast both + ways.

+
+

The row rules

+
    +
  1. + Two genres. A content arrival (SourcePostAdded, + SquadPostAdded, UserPostAdded) makes the article headline the payload + — the sentence announcing it is boilerplate that repeats on every row. + A social row makes the person the payload. The row leads with + whichever it is. +
  2. +
  3. + Attribution comes from the avatars, not the copy. A content + arrival shows The New Stack or Luffy in AI{' '} + under the headline — never "New post in …", which carries no + information inside the notifications inbox. +
  4. +
  5. + The timestamp follows the last grey line —{' '} + The New Stack · 3h,{' '} + Scaling our cache layer · 5h. A row with no grey text at + all gets it on a line of its own. It never rides the leading line. +
  6. +
+

+ Live examples of all four shapes: Notifications / List item — all types + / Row anatomy. +

+
+
{surfaces.map((row, index) => (
(

Shared readability levers

  • - Type scale: titles use typo-callout bold; meta uses{' '} - typo-footnote. Most "hard to read" reports trace - back to the footnote meta line at text-tertiary /{' '} - text-quaternary. + Type scale: the leading line uses typo-callout; grey + lines use typo-subhead / typo-footnote at{' '} + text-tertiary. The original "hard to read" + report was not about contrast — it was the headline sitting in the + grey tier while boilerplate held the primary one. +
  • +
  • + A line the timestamp rides must be a single-line truncate{' '} + flex row with the time as a sibling. multi-truncate is{' '} + display: -webkit-box and blockifies as a flex item, + losing its clamp and width; a global{' '} + + * {'{'} flex-shrink: 0 {'}'} + {' '} + means the text must opt back into shrinking or it overflows the row.
  • Color tokens only (text-primary,{' '} diff --git a/packages/storybook/stories/components/notifications/UseCases.stories.tsx b/packages/storybook/stories/components/notifications/UseCases.stories.tsx index 21a77774a08..c5ccefb96a0 100644 --- a/packages/storybook/stories/components/notifications/UseCases.stories.tsx +++ b/packages/storybook/stories/components/notifications/UseCases.stories.tsx @@ -30,7 +30,7 @@ const meta: Meta = { docs: { description: { component: - 'Every notification scenario with its avatar/badge rationale. Lead avatar rule: always show the human who acted (commenter, upvoter, follower, poster); fall back to the source only when there is no person; system/digest rows show the type icon.', + 'Every notification scenario with its avatar/badge rationale. Lead avatar rule: always show the human who acted (commenter, upvoter, follower, poster); fall back to the source only when there is no person; system/digest rows show the type icon. This page is about the avatar column only — for which line leads and where the timestamp lands, see "Row anatomy".', }, }, },