From 352f7317f8f50b828dec5f77d585fc545dfc7ad2 Mon Sep 17 00:00:00 2001 From: Jamie B <53781962+JamieB-gu@users.noreply.github.com> Date: Wed, 25 Mar 2026 18:19:24 +0000 Subject: [PATCH] Refactor `LiveblogNotifications` We want to expand usage of the notifications button to include subscribing to football match events. This refactor makes the button more generic and testable, to facilitate this. Firstly the component has been renamed to `NotificationsToggle`, so that it can be reused for both liveblog notifications (new content) and football notifications (match events such as goals). This component contains the logic for user events, calling Bridget, and synchronising UI with the subscription state. The UI of the button itself is handled in a new, presentational component called `ToggleButton`, which replaces `FollowNotificationsButton` and is more suitable for customisation to the new football design. `FollowNotificationsButton` will soon be deleted, as its only other usage is in the process of being deprecated in a separate change. Wrapping `NotificationsToggle` is a lightweight island component that dependency-injects the Bridget client. This allows us to use a "live" implementation of this client in production, and a mock implementation in tests and stories. As a result, this change also adds a story containing UI tests that make use of a mock Bridget client. The type describing the API of the `Notifications` Bridget client has been simplified to make dependency injection and mocking simpler. A similar type can be rolled out for the other Bridget clients in a future change. --- .../src/components/ArticleMeta.apps.tsx | 50 ++++-- .../LiveblogNotifications.importable.tsx | 117 -------------- .../NotificationsToggle.importable.tsx | 15 ++ .../NotificationsToggle.stories.tsx | 75 +++++++++ .../src/components/NotificationsToggle.tsx | 145 ++++++++++++++++++ .../src/components/ToggleButton.tsx | 86 +++++++++++ dotcom-rendering/src/lib/bridgetApi.ts | 15 +- 7 files changed, 372 insertions(+), 131 deletions(-) delete mode 100644 dotcom-rendering/src/components/LiveblogNotifications.importable.tsx create mode 100644 dotcom-rendering/src/components/NotificationsToggle.importable.tsx create mode 100644 dotcom-rendering/src/components/NotificationsToggle.stories.tsx create mode 100644 dotcom-rendering/src/components/NotificationsToggle.tsx create mode 100644 dotcom-rendering/src/components/ToggleButton.tsx diff --git a/dotcom-rendering/src/components/ArticleMeta.apps.tsx b/dotcom-rendering/src/components/ArticleMeta.apps.tsx index fe34af72e1e..94d872f168d 100644 --- a/dotcom-rendering/src/components/ArticleMeta.apps.tsx +++ b/dotcom-rendering/src/components/ArticleMeta.apps.tsx @@ -23,7 +23,7 @@ import { Dateline } from './Dateline'; import { FollowWrapper } from './FollowWrapper.importable'; import { Island } from './Island'; import { ListenToArticle } from './ListenToArticle.importable'; -import { LiveblogNotifications } from './LiveblogNotifications.importable'; +import { NotificationsToggle } from './NotificationsToggle.importable'; type Props = { format: ArticleFormat; @@ -249,9 +249,6 @@ export const ArticleMetaApps = ({ const shouldShowFollowButtons = (layoutOrDesignType: boolean) => layoutOrDesignType && !!byline && !isUndefined(soleContributor); - const shouldShowLiveblogNotifications = - isLiveBlog && !!pageId && !!headline; - const isImmersiveOrAnalysisWithMultipleAuthors = (isAnalysis || isImmersive) && !!byline && isUndefined(soleContributor); @@ -311,14 +308,11 @@ export const ArticleMetaApps = ({ /> )} - {shouldShowLiveblogNotifications && ( - - - - )} + {isCommentable && ( ); }; + +const LiveblogNotifications = (props: { + isLiveBlog: boolean; + headline: string | undefined; + pageId: string | undefined; +}) => + props.isLiveBlog && !!props.pageId && !!props.headline ? ( +
+ + + +
+ ) : null; diff --git a/dotcom-rendering/src/components/LiveblogNotifications.importable.tsx b/dotcom-rendering/src/components/LiveblogNotifications.importable.tsx deleted file mode 100644 index 93cc57abe76..00000000000 --- a/dotcom-rendering/src/components/LiveblogNotifications.importable.tsx +++ /dev/null @@ -1,117 +0,0 @@ -import { css } from '@emotion/react'; -import { Topic } from '@guardian/bridget/Topic'; -import { isUndefined, log } from '@guardian/libs'; -import { from, space } from '@guardian/source/foundations'; -import { useEffect, useState } from 'react'; -import { getNotificationsClient } from '../lib/bridgetApi'; -import { FollowNotificationsButton } from './FollowButtons'; - -type Props = { - id: string; - displayName: string; -}; - -export const LiveblogNotifications = ({ id, displayName }: Props) => { - const [isFollowingNotifications, setIsFollowingNotifications] = useState< - boolean | undefined - >(undefined); - - useEffect(() => { - const topic = new Topic({ - id, - displayName, - type: 'content', - }); - - void getNotificationsClient() - .isFollowing(topic) - .then(setIsFollowingNotifications) - .catch((error) => { - window.guardian.modules.sentry.reportError( - error, - 'bridget-getNotificationsClient-isFollowing-error', - ); - log( - 'dotcom', - 'Bridget getNotificationsClient.isFollowing Error:', - error, - ); - }); - }, [id, displayName]); - - const notificationsHandler = () => { - const topic = new Topic({ - id, - displayName, - type: 'content', - }); - - if (isFollowingNotifications) { - void getNotificationsClient() - .unfollow(topic) - .then((success) => { - if (success) { - setIsFollowingNotifications(false); - } - }) - .catch((error) => { - window.guardian.modules.sentry.reportError( - error, - 'briidget-getNotificationsClient-unfollow-error', - ); - log( - 'dotcom', - 'Bridget getNotificationsClient.unfollow Error:', - error, - ); - }); - } else { - void getNotificationsClient() - .follow(topic) - .then((success) => { - if (success) { - setIsFollowingNotifications(true); - } - }) - .catch((error) => { - window.guardian.modules.sentry.reportError( - error, - 'bridget-getNotificationsClient-follow-error', - ); - log( - 'dotcom', - 'Bridget getNotificationsClient.follow Error:', - error, - ); - }); - } - }; - - return ( -
- undefined - } - /> -
- ); -}; diff --git a/dotcom-rendering/src/components/NotificationsToggle.importable.tsx b/dotcom-rendering/src/components/NotificationsToggle.importable.tsx new file mode 100644 index 00000000000..936010e2a35 --- /dev/null +++ b/dotcom-rendering/src/components/NotificationsToggle.importable.tsx @@ -0,0 +1,15 @@ +import type { ComponentProps } from 'react'; +import { getNotificationsClient } from '../lib/bridgetApi'; +import { NotificationsToggle as NotificationsToggleComponent } from './NotificationsToggle'; + +type Props = Omit< + ComponentProps, + 'notificationsClient' +>; + +export const NotificationsToggle = (props: Props) => ( + +); diff --git a/dotcom-rendering/src/components/NotificationsToggle.stories.tsx b/dotcom-rendering/src/components/NotificationsToggle.stories.tsx new file mode 100644 index 00000000000..bb9bd9070d5 --- /dev/null +++ b/dotcom-rendering/src/components/NotificationsToggle.stories.tsx @@ -0,0 +1,75 @@ +import { Topic } from '@guardian/bridget'; +import type { Meta, StoryObj } from '@storybook/react-webpack5'; +import { expect, fn, userEvent, within } from 'storybook/test'; +import type { NotificationsClient } from '../lib/bridgetApi'; +import { NotificationsToggle as NotificationsToggleComponent } from './NotificationsToggle'; + +const meta = { + component: NotificationsToggleComponent, +} satisfies Meta; + +export default meta; + +type Story = StoryObj; + +const mockNotificationsClient: NotificationsClient = (() => { + let following = false; + + return { + follow: fn(() => { + following = true; + return Promise.resolve(true); + }), + + unfollow: fn(() => { + following = false; + return Promise.resolve(true); + }), + + isFollowing: fn(() => { + return Promise.resolve(following); + }), + }; +})(); + +export const NotificationsToggle = { + args: { + displayName: 'A notification', + id: 'a-notification-id', + notificationType: 'content', + notificationsClient: mockNotificationsClient, + }, + play: async ({ args, step, canvasElement }) => { + const canvas = within(canvasElement); + const button = canvas.getByRole('button'); + const expectedTopic = new Topic({ + displayName: args.displayName, + id: args.id, + type: args.notificationType, + }); + + await expect(button).toHaveTextContent('Notifications off'); + + await step('isFollowing is called', async () => { + await expect( + mockNotificationsClient.isFollowing, + ).toHaveBeenCalledWith(expectedTopic); + }); + + await step('follow is called when button is clicked', async () => { + await userEvent.click(button); + await expect(mockNotificationsClient.follow).toHaveBeenCalledWith( + expectedTopic, + ); + await expect(button).toHaveTextContent('Notifications on'); + }); + + await step('unfollow is called when button is clicked', async () => { + await userEvent.click(button); + await expect(mockNotificationsClient.unfollow).toHaveBeenCalledWith( + expectedTopic, + ); + await expect(button).toHaveTextContent('Notifications off'); + }); + }, +} satisfies Story; diff --git a/dotcom-rendering/src/components/NotificationsToggle.tsx b/dotcom-rendering/src/components/NotificationsToggle.tsx new file mode 100644 index 00000000000..11de015bc9f --- /dev/null +++ b/dotcom-rendering/src/components/NotificationsToggle.tsx @@ -0,0 +1,145 @@ +import { Topic } from '@guardian/bridget/Topic'; +import { log } from '@guardian/libs'; +import { + SvgNotificationsOff, + SvgNotificationsOn, +} from '@guardian/source/react-components'; +import { useEffect, useState } from 'react'; +import type { NotificationsClient } from '../lib/bridgetApi'; +import { ToggleButton } from './ToggleButton'; + +type Props = { + id: string; + displayName: string; + notificationType: 'content'; + notificationsClient: NotificationsClient; +}; + +export const NotificationsToggle = ({ + id, + displayName, + notificationType, + notificationsClient, +}: Props) => { + const [isFollowing, setIsFollowing] = useIsFollowing( + id, + displayName, + notificationType, + notificationsClient, + ); + + return ( + + ) : ( + + ) + } + iconBackground={isFollowing ? '--follow-icon-fill' : undefined} + iconBorder="--follow-icon-fill" + iconFill={ + isFollowing ? '--follow-icon-background' : '--follow-icon-fill' + } + onClick={toggleNotifications( + id, + displayName, + notificationType, + notificationsClient, + isFollowing, + setIsFollowing, + )} + > + Notifications {isFollowing ? 'on' : 'off'} + + ); +}; + +/** + * Retrieves information about whether the user is following a particular topic, + * via Bridget, and stores it in a state variable for use in the rest of the + * component. Also supplies a setter to update that state, similar to + * `useState`. + */ +const useIsFollowing = ( + id: string, + displayName: string, + notificationType: Props['notificationType'], + notificationsClient: Props['notificationsClient'], +): [ + boolean | undefined, + React.Dispatch>, +] => { + const [isFollowing, setIsFollowing] = useState( + undefined, + ); + + useEffect(() => { + const topic = new Topic({ + id, + displayName, + type: notificationType, + }); + + void notificationsClient + .isFollowing(topic) + .then(setIsFollowing) + .catch(handleError('isFollowing')); + }, [id, displayName, notificationType, notificationsClient]); + + return [isFollowing, setIsFollowing]; +}; + +const toggleNotifications = ( + id: string, + displayName: string, + notificationType: Props['notificationType'], + notificationsClient: Props['notificationsClient'], + isFollowing: boolean | undefined, + setIsFollowing: (a: boolean) => void, +): (() => void) => + isFollowing === undefined + ? () => undefined + : () => { + const topic = new Topic({ + id, + displayName, + type: notificationType, + }); + + if (isFollowing) { + void notificationsClient + .unfollow(topic) + .then((success) => { + if (success) { + setIsFollowing(false); + } + }) + .catch(handleError('unfollow')); + } else { + void notificationsClient + .follow(topic) + .then((success) => { + if (success) { + setIsFollowing(true); + } + }) + .catch(handleError('follow')); + } + }; + +const handleError = + (methodName: 'follow' | 'unfollow' | 'isFollowing') => + (error: any): void => { + window.guardian.modules.sentry.reportError( + error, + `bridget-getNotificationsClient-${methodName}-error`, + ); + log( + 'dotcom', + `Bridget getNotificationsClient.${methodName} Error:`, + error, + ); + }; diff --git a/dotcom-rendering/src/components/ToggleButton.tsx b/dotcom-rendering/src/components/ToggleButton.tsx new file mode 100644 index 00000000000..c0b2c255a4a --- /dev/null +++ b/dotcom-rendering/src/components/ToggleButton.tsx @@ -0,0 +1,86 @@ +import { space, textSans15Object } from '@guardian/source/foundations'; +import type { ButtonHTMLAttributes, ReactNode } from 'react'; +import { palette } from '../palette'; +import type { ColourName } from '../paletteDeclarations'; + +type Props = { + onClick: () => void; + children: ReactNode; + colour: ColourName; + icon: ReactNode; + iconFill: ColourName; + iconBackground: ColourName | undefined; + iconBorder: ColourName; +}; + +export const ToggleButton = (props: Props) => ( + +); + +const Button = (props: { + onClick: ButtonHTMLAttributes['onClick']; + children: ReactNode; + colour: ColourName; +}) => ( + +); + +const Icon = (props: { + fill: ColourName; + background: ColourName | undefined; + border: ColourName; + children: ReactNode; +}) => ( +
+ {props.children} +
+); diff --git a/dotcom-rendering/src/lib/bridgetApi.ts b/dotcom-rendering/src/lib/bridgetApi.ts index 644fb63e608..a20811faff6 100644 --- a/dotcom-rendering/src/lib/bridgetApi.ts +++ b/dotcom-rendering/src/lib/bridgetApi.ts @@ -1,3 +1,4 @@ +import type { ThriftClient } from '@creditkarma/thrift-server-core'; import * as AbTesting from '@guardian/bridget/AbTesting'; import * as Acquisitions from '@guardian/bridget/Acquisitions'; import * as Analytics from '@guardian/bridget/Analytics'; @@ -18,6 +19,15 @@ import * as Video from '@guardian/bridget/Videos'; import { isUndefined } from '@guardian/libs'; import { createAppClient } from './thrift/nativeConnection'; +type BridgetClient = Omit< + Client, + | '_serviceName' + | '_annotations' + | '_methodAnnotations' + | '_methodNames' + | '_methodParameters' +>; + let environmentClient: Environment.Client | undefined = undefined; export const getEnvironmentClient = (): Environment.Client => { if (isUndefined(environmentClient)) { @@ -54,8 +64,9 @@ export const getAcquisitionsClient = (): Acquisitions.Client => { return acquisitionsClient; }; -let notificationsClient: Notifications.Client | undefined = undefined; -export const getNotificationsClient = (): Notifications.Client => { +export type NotificationsClient = BridgetClient; +let notificationsClient: NotificationsClient | undefined = undefined; +export const getNotificationsClient = (): NotificationsClient => { if (!notificationsClient) { notificationsClient = createAppClient>( Notifications.Client,