14890 show or hide the sign up newsletter component #14929
Conversation
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
…nto 14890-show-or-hide-the-sign-up-newsletter-component
…nto 14890-show-or-hide-the-sign-up-newsletter-component
…nto 14890-show-or-hide-the-sign-up-newsletter-component
| }: EmailSignUpWrapperProps) => { | ||
| const [idApiUrl] = useState(() => { | ||
| if (typeof window === 'undefined') return ''; | ||
| return window.guardian?.config?.page?.idApiUrl ?? ''; |
There was a problem hiding this comment.
Might it be better to return undefined rather than an empty string here and then make the type string | undefined?
…nto 14890-show-or-hide-the-sign-up-newsletter-component
…ttps://github.com/guardian/dotcom-rendering into 14890-show-or-hide-the-sign-up-newsletter-component
…te the type comparision
georgerichmond
left a comment
georgerichmond
left a comment
There was a problem hiding this comment.
Looks good - just wondering about how we best test this - perhaps using the hook in the story is useful rather than repeating the content?
| /** | ||
| * A hook to check if a user is subscribed to a specific newsletter. | ||
| */ | ||
| export const useNewsletterSubscription = ( |
There was a problem hiding this comment.
Is it worth having a test for this?
There was a problem hiding this comment.
It could be tested as part of testing the component by mocking the fetch call possibly
| hidePrivacyMessage?: boolean; | ||
| } | ||
|
|
||
| const AlreadySubscribedWrapper = (props: EmailSignUpWrapperProps) => { |
There was a problem hiding this comment.
Could we use the hook in the react story and just mock out what the service call returns?
There was a problem hiding this comment.
Either by mocking fetch or if there is a standard way it's mocked out in the codebase so that we can see the hook being used in the story.
Otherwise it's just repeating logic from the hook for the point of the story - may as well just show the component as it looks with and without a subscription
|
I couldn't see the story in storybook for some reason |
…nto 14890-show-or-hide-the-sign-up-newsletter-component
I'm able to see the new storybook, these are the command that i'm using
|
| } | ||
| } | ||
|
|
||
| const mockAccessToken = { |
There was a problem hiding this comment.
Do we have to mock all this? Is it not possible to just mock the useAuthStatus hook?
There was a problem hiding this comment.
(Note, this is a reply to George's comment above, but the GH review UI might not show this clearly).
Doesn't the useAuthStatus hook also return this data via the SignedIn type though? I can also see that the SecureSignup component relies on a few different functions/hooks to retrieve information about the signed in status and email address, such as lazyFetchEmailWithTimeout and useIsSignedIn, which in turn rely on this?
An alternative might be to use storybook's built-in mocking1 for the modules in question? Given that you've already got tests for useNewsletterSubscription we probably don't need to re-test the underlying implementation in storybook, as we just want to test the resulting component behaviour. Therefore we could add the following to storybook's preview.ts to set up mocking for the relevant modules:
sb.mock(import('../src/lib/useNewsletterSubscription.ts'), { spy: true });
sb.mock(import('../src/lib/fetchEmail.ts'), { spy: true });
sb.mock(import('../src/lib/useAuthStatus.ts'), { spy: true });Then we can use beforeEach in storybook2 to mock the specific functionality we care about. So, for each story:
LoadingState:
async beforeEach() {
mocked(useNewsletterSubscription).mockReturnValue(undefined);
},DefaultStory/DefaultStoryWithPrivacy:
async beforeEach() {
mocked(useNewsletterSubscription).mockReturnValue(false);
},SignedInNotSubscribed:
async beforeEach() {
mocked(useNewsletterSubscription).mockReturnValue(false);
mocked(lazyFetchEmailWithTimeout).mockReturnValue(() => Promise.resolve("mock email"));
mocked(useIsSignedIn).mockReturnValue(true);
},SignedInAlreadySubscribed:
async beforeEach() {
mocked(useNewsletterSubscription).mockReturnValue(true);
},I think this would allow you to avoid writing custom mocks for the identity module, and the auth decorators too. However, I doubt we have other examples of this in the codebase, as I believe it's a relatively recent feature (Storybook 9), so I'd be happy to discuss if helpful.
Footnotes
|
Looks great, was just wondering if it was simpler to mock the useAuthStatus hook rather than everything that it uses on the window? |
| authStatus.kind === 'SignedIn' ? true : false, | ||
| ); | ||
|
|
||
| export const getAuthStatus = async (): Promise<AuthStatus> => { |
There was a problem hiding this comment.
I think this is the only one that's actually used - would it be simpler to just mock this instead
| if (isSubscribed === true) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
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.
|
|
||
| setIsSubscribed(isUserSubscribed); | ||
| } catch (error) { | ||
| console.error('Error fetching newsletters:', error); |
There was a problem hiding this comment.
Rather than console.error, we have a Sentry reportError function on the window which will allow you to report errors to Sentry. Here's an example e.g.:
There was a problem hiding this comment.
I should have said earlier, any access of window should be inside a useEffect because:
a) that is the right place for side-effects h/t @JamieB-gu
b) so there is no risk of the component being used in a server render and causing an exception in the main render path
…nto 14890-show-or-hide-the-sign-up-newsletter-component
Good question! I tried mocking useAuthStatus directly, but Storybook doesn’t support jest.mock() since it runs in a real browser. The hook reads the actual auth state from the browser, so we can’t override it that way. Using Webpack’s resolve.alias is the recommended approach — it lets us cleanly swap the identity module and control the auth state per story. |
JamieB-gu
left a comment
JamieB-gu
left a comment
There was a problem hiding this comment.
Looks good! 👍 for the tests and JSDocs.
A few questions and comments, one related to @georgerichmond 's point above.
| Origin: origin, | ||
| Referer: origin, |
| const PLACEHOLDER_HEIGHTS = new Map([ | ||
| ['mobile', 220], | ||
| ['tablet', 180], | ||
| ['desktop', 180], | ||
| ] as const) as Map<'mobile' | 'tablet' | 'desktop', number>; |
There was a problem hiding this comment.
If you pass these as type parameters to the constructor instead I think you can avoid the type cast?
| const PLACEHOLDER_HEIGHTS = new Map([ | |
| ['mobile', 220], | |
| ['tablet', 180], | |
| ['desktop', 180], | |
| ] as const) as Map<'mobile' | 'tablet' | 'desktop', number>; | |
| const PLACEHOLDER_HEIGHTS = new Map<'mobile' | 'tablet' | 'desktop', number>([ | |
| ['mobile', 220], | |
| ['tablet', 180], | |
| ['desktop', 180], | |
| ]); |
This will allow TS to check that the type is correct. With the type cast, if one of the properties were to be misspelled, for example "mobil", the compiler won't be able to tell you. Alternatively, you could import the Breakpoint type for this, as that's what the Placeholder component uses itself:
| const PLACEHOLDER_HEIGHTS = new Map([ | |
| ['mobile', 220], | |
| ['tablet', 180], | |
| ['desktop', 180], | |
| ] as const) as Map<'mobile' | 'tablet' | 'desktop', number>; | |
| const PLACEHOLDER_HEIGHTS = new Map<Breakpoint, number>([ | |
| ['mobile', 220], | |
| ['tablet', 180], | |
| ['desktop', 180], | |
| ]); |
| }: EmailSignUpWrapperProps) => { | ||
| const [idApiUrl] = useState(() => { | ||
| if (typeof window === 'undefined') return undefined; | ||
| return window.guardian?.config?.page?.idApiUrl ?? undefined; |
There was a problem hiding this comment.
useState requires its initialiser function to be pure1, and looking up values on the window object is technically a side-effect because the window is a global, mutable piece of state. In addition, this component will be server-rendered and the window object isn't available there, so we may need to look for an alternative approach to retrieve this value?
I think we have two options. The first would be to pass the idApiUrl as a prop to this component. It's available on the server, so can be prop-drilled down, and will then be automatically available on the client via islands. You can see examples of this elsewhere in the codebase where this value is used. I think this is the preferred approach, although it will involve prop-drilling through a few components.
The other option would be to continue accessing this value on the window, but inside a useEffect, as that's the place to run side-effects. It's also safe for server rendering, as the function passed to useEffect won't be run there2. I don't think this is the best option in this case, as it adds some complexity to the component, and we can already make this data available through props as mentioned.
Let me know if you'd like to discuss!
Footnotes
| } | ||
| } | ||
|
|
||
| const mockAccessToken = { |
There was a problem hiding this comment.
(Note, this is a reply to George's comment above, but the GH review UI might not show this clearly).
Doesn't the useAuthStatus hook also return this data via the SignedIn type though? I can also see that the SecureSignup component relies on a few different functions/hooks to retrieve information about the signed in status and email address, such as lazyFetchEmailWithTimeout and useIsSignedIn, which in turn rely on this?
An alternative might be to use storybook's built-in mocking1 for the modules in question? Given that you've already got tests for useNewsletterSubscription we probably don't need to re-test the underlying implementation in storybook, as we just want to test the resulting component behaviour. Therefore we could add the following to storybook's preview.ts to set up mocking for the relevant modules:
sb.mock(import('../src/lib/useNewsletterSubscription.ts'), { spy: true });
sb.mock(import('../src/lib/fetchEmail.ts'), { spy: true });
sb.mock(import('../src/lib/useAuthStatus.ts'), { spy: true });Then we can use beforeEach in storybook2 to mock the specific functionality we care about. So, for each story:
LoadingState:
async beforeEach() {
mocked(useNewsletterSubscription).mockReturnValue(undefined);
},DefaultStory/DefaultStoryWithPrivacy:
async beforeEach() {
mocked(useNewsletterSubscription).mockReturnValue(false);
},SignedInNotSubscribed:
async beforeEach() {
mocked(useNewsletterSubscription).mockReturnValue(false);
mocked(lazyFetchEmailWithTimeout).mockReturnValue(() => Promise.resolve("mock email"));
mocked(useIsSignedIn).mockReturnValue(true);
},SignedInAlreadySubscribed:
async beforeEach() {
mocked(useNewsletterSubscription).mockReturnValue(true);
},I think this would allow you to avoid writing custom mocks for the identity module, and the auth decorators too. However, I doubt we have other examples of this in the codebase, as I believe it's a relatively recent feature (Storybook 9), so I'd be happy to discuss if helpful.
Footnotes
…nto 14890-show-or-hide-the-sign-up-newsletter-component
…ttps://github.com/guardian/dotcom-rendering into 14890-show-or-hide-the-sign-up-newsletter-component
|
Looking much better, simpler just to mock the hook 👍 |
What does this change?
Investigate hiding the sign up component
Why?
There is an ambition to hide the sign up component that appears in articles when a user has already signed up to the newsletter in question. This would be a nice "quality of life" update that should make the sign up embeds less intrusive.
Ticket link: #14889
Subtask ticket link : #14890
Screenshots