Open
Conversation
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.
Member
|
|
If you have two component files with similar names, but one includes `.island`, the "name" attribute in the "gu-island" element doesn't match the file name (extra characters get added). This means that when the hydration code attempts to import the island JS, using this name, it cannot find the file. The fix for now is to give the two components distinct names.
JamieB-gu
added
run_chromatic
github-actions
bot
removed
the
run_chromatic
github-actions
bot
removed
the
run_chromatic
arelra
reviewed
Apr 1, 2026
| notificationsClient: Props['notificationsClient'], | ||
| ): [ | ||
| boolean | undefined, | ||
| React.Dispatch<React.SetStateAction<boolean | undefined>>, |
Member
There was a problem hiding this comment.
Minor non-blocking. Perhaps a project-wide utility type would be useful e.g.
type SetState<T> = React.Dispatch<React.SetStateAction<T>>;
arelra
reviewed
Apr 1, 2026
| } | ||
| }; | ||
|
|
||
| const handleError = |
Member
There was a problem hiding this comment.
Minor non-blocking. A version of this would be another useful library function.
I see a similar somewhat similar function here:
dotcom-rendering/dotcom-rendering/src/components/SelfHostedVideo.island.tsx
Lines 141 to 154 in 660fd8d
arelra
approved these changes
Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 replacesFollowNotificationsButtonand is more suitable for customisation to the new football design.FollowNotificationsButtonwill soon be deleted, as its only other usage is in the process of being deprecated in a separate change (#15558).Wrapping
NotificationsToggleis 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
NotificationsBridget 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.Part of #14905.