-
Notifications
You must be signed in to change notification settings - Fork 30
14890 show or hide the sign up newsletter component #14929
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.
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
base: main
Are you sure you want to change the base?
Changes from 21 commits
74f916e
6c6ffc1
177412b
b999d89
da6e644
a5b9c4b
7ad7a22
6f12d91
bb54b31
4927841
8582503
b56c69f
618f558
08ba7cc
6e620a5
8eed71c
97e519d
f0de441
d5dbdaa
6161c89
67ce48a
c190059
0a9eb75
c2f8825
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,26 @@ | ||
| import type { Meta, StoryObj } from '@storybook/react-webpack5'; | ||
| import { EmailSignUpWrapper } from './EmailSignUpWrapper'; | ||
| import { useEffect } from 'react'; | ||
| import type { EmailSignUpProps } from './EmailSignup'; | ||
| import { EmailSignUpWrapper } from './EmailSignUpWrapper.importable'; | ||
|
|
||
| const meta: Meta<typeof EmailSignUpWrapper> = { | ||
| title: 'Components/EmailSignUpWrapper', | ||
| component: EmailSignUpWrapper, | ||
| }; | ||
|
|
||
| type Story = StoryObj<typeof EmailSignUpWrapper>; | ||
|
|
||
| const defaultArgs = { | ||
| index: 10, | ||
| listId: 4147, | ||
| identityName: 'the-recap', | ||
| description: | ||
| 'The best of our sports journalism from the past seven days and a heads-up on the weekend’s action', | ||
| "The best of our sports journalism from the past seven days and a heads-up on the weekend's action", | ||
| name: 'The Recap', | ||
| frequency: 'Weekly', | ||
| successDescription: "We'll send you The Recap every week", | ||
| theme: 'sport', | ||
| } satisfies Story['args']; | ||
| type Story = StoryObj<typeof EmailSignUpWrapper>; | ||
|
|
||
| export const DefaultStory: Story = { | ||
| args: { | ||
|
|
@@ -32,4 +36,55 @@ export const DefaultStoryWithPrivacy: Story = { | |
| }, | ||
| }; | ||
|
|
||
| // Simulates when user is NOT subscribed - signup form is visible | ||
| export const NotSubscribedState: Story = { | ||
| args: { | ||
| hidePrivacyMessage: false, | ||
| ...defaultArgs, | ||
| }, | ||
| decorators: [(Story) => <Story />], | ||
| }; | ||
|
|
||
| // Simulates when user IS subscribed - component returns null | ||
| interface EmailSignUpWrapperProps extends EmailSignUpProps { | ||
| index: number; | ||
| listId: number; | ||
| identityName: string; | ||
| successDescription: string; | ||
| hidePrivacyMessage?: boolean; | ||
| } | ||
|
|
||
| const AlreadySubscribedWrapper = (props: EmailSignUpWrapperProps) => { | ||
|
||
| // Directly use the hook logic - in a real scenario it would return true | ||
| // For storybook, we simulate by just returning null | ||
| const isSubscribed = true; // Simulating hook return value | ||
|
|
||
| useEffect(() => { | ||
| const idApiUrl = | ||
| typeof window !== 'undefined' | ||
| ? window.guardian?.config?.page?.idApiUrl | ||
| : undefined; | ||
| console.log( | ||
| 'AlreadySubscribedState: useNewsletterSubscription would return true', | ||
| ); | ||
| console.log('Props:', { listId: props.listId, idApiUrl }); | ||
| }, [props.listId]); | ||
|
|
||
| if (isSubscribed) { | ||
| return null; | ||
| } | ||
|
|
||
| return <EmailSignUpWrapper {...props} />; | ||
| }; | ||
|
|
||
| export const AlreadySubscribedState = { | ||
| render: (args: EmailSignUpWrapperProps) => ( | ||
| <AlreadySubscribedWrapper {...args} /> | ||
| ), | ||
| args: { | ||
| ...defaultArgs, | ||
| hidePrivacyMessage: false, | ||
| }, | ||
| } satisfies Story; | ||
|
|
||
| export default meta; | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,93 @@ | ||||||||||
| import { useEffect, useState } from 'react'; | ||||||||||
| import { getOptionsHeaders } from './identity'; | ||||||||||
| import { useAuthStatus } from './useAuthStatus'; | ||||||||||
|
|
||||||||||
| interface NewsletterSubscriptionResponse { | ||||||||||
| result: { | ||||||||||
| subscriptions: Array<{ | ||||||||||
| listId: string; | ||||||||||
| }>; | ||||||||||
| }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * A hook to check if a user is subscribed to a specific newsletter. | ||||||||||
| */ | ||||||||||
| export const useNewsletterSubscription = ( | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth having a test for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be tested as part of testing the component by mocking the fetch call possibly |
||||||||||
| newsletterId: number, | ||||||||||
| idApiUrl: string | undefined, | ||||||||||
| ): boolean | undefined => { | ||||||||||
| const [isSubscribed, setIsSubscribed] = useState<boolean | undefined>( | ||||||||||
| undefined, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| const authStatus = useAuthStatus(); | ||||||||||
|
|
||||||||||
| useEffect(() => { | ||||||||||
| // Wait for auth to be determined | ||||||||||
| if (authStatus.kind === 'Pending') { | ||||||||||
| setIsSubscribed(undefined); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // User is not signed in | ||||||||||
| if (authStatus.kind === 'SignedOut') { | ||||||||||
| setIsSubscribed(false); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // No API URL available | ||||||||||
| if (idApiUrl === undefined || idApiUrl === '') { | ||||||||||
| setIsSubscribed(false); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // At this point: authStatus.kind === 'SignedIn' | ||||||||||
|
|
||||||||||
| // User is signed in, fetch their newsletters | ||||||||||
| const fetchNewsletters = async () => { | ||||||||||
| try { | ||||||||||
georgerichmond marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| const options = getOptionsHeaders(authStatus); | ||||||||||
| const origin = window.location.origin; | ||||||||||
| const response = await fetch( | ||||||||||
| `${idApiUrl}/users/me/newsletters`, | ||||||||||
| { | ||||||||||
| method: 'GET', | ||||||||||
| credentials: 'include', | ||||||||||
| ...options, | ||||||||||
| headers: { | ||||||||||
| ...options.headers, | ||||||||||
| Origin: origin, | ||||||||||
| Referer: origin, | ||||||||||
| }, | ||||||||||
| }, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| if (!response.ok) { | ||||||||||
| console.error('Failed to fetch user newsletters'); | ||||||||||
| setIsSubscribed(false); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const data: NewsletterSubscriptionResponse = | ||||||||||
| await response.json(); | ||||||||||
|
|
||||||||||
| const newsletters = data.result?.subscriptions ?? []; | ||||||||||
|
|
||||||||||
| // If newsletter exists in the subscriptions array, user is subscribed | ||||||||||
| const isUserSubscribed = newsletters.some( | ||||||||||
| (n) => Number(n.listId) === newsletterId, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| setIsSubscribed(isUserSubscribed); | ||||||||||
| } catch (error) { | ||||||||||
| console.error('Error fetching newsletters:', error); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than console.error, we have a Sentry dotcom-rendering/dotcom-rendering/src/components/StickyBottomBanner/ReaderRevenueBanner.tsx Lines 98 to 101 in 9038dfd
|
||||||||||
| setIsSubscribed(false); | ||||||||||
| } | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| void fetchNewsletters(); | ||||||||||
| }, [authStatus, newsletterId, idApiUrl]); | ||||||||||
|
|
||||||||||
| return isSubscribed; | ||||||||||
| }; | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we initially render the the newsletter component and then remove it if we determine there is a subscription?
I assume we have considered this approach will cause the least layout shift vs not showing and then inserting.
There is also the possibility of showing a newsletter related placeholder and replacing with the aim of zero layout shift.