Skip to content

Commit 2b10a96

Browse files
vbudhramvbudhram
committed
fix(settings): fix Signin CMS config selection and conditionally render description
Simplify isCmsHeader predicate and fix CMS config selection for Signin pages. Conditionally render the CMS description <p> tag only when cmsDescription is defined, avoiding empty elements when Product omits the description field.
1 parent 5d30869 commit 2b10a96

7 files changed

Lines changed: 77 additions & 43 deletions

File tree

‎packages/fxa-settings/src/components/CardHeader/index.test.tsx‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,25 +87,33 @@ describe('CardHeader', () => {
8787
).toBeInTheDocument();
8888
});
8989

90-
it('renders CMS header with only description', () => {
90+
it('falls back to default heading when only description is provided (no headline)', () => {
9191
renderWithLocalizationProvider(
9292
<CardHeader
9393
headingText={MOCK_HEADING}
9494
cmsDescription={MOCK_CMS_DESCRIPTION}
9595
/>
9696
);
97-
expect(screen.getByText(MOCK_CMS_DESCRIPTION)).toBeInTheDocument();
97+
// Without cmsHeadline, the CMS path is not used
98+
expect(
99+
screen.getByRole('heading', { name: MOCK_HEADING })
100+
).toBeInTheDocument();
101+
expect(screen.queryByText(MOCK_CMS_DESCRIPTION)).not.toBeInTheDocument();
98102
});
99103

100-
it('renders CMS header with only logo', () => {
104+
it('falls back to default heading when only logo is provided (no headline)', () => {
101105
renderWithLocalizationProvider(
102106
<CardHeader
103107
headingText={MOCK_HEADING}
104108
cmsLogoUrl={MOCK_CMS_LOGO_URL}
105109
cmsLogoAltText={MOCK_CMS_LOGO_ALT_TEXT}
106110
/>
107111
);
108-
expect(screen.getByAltText(MOCK_CMS_LOGO_ALT_TEXT)).toBeInTheDocument();
112+
// Without cmsHeadline, the CMS path is not used
113+
expect(
114+
screen.getByRole('heading', { name: MOCK_HEADING })
115+
).toBeInTheDocument();
116+
expect(screen.queryByAltText(MOCK_CMS_LOGO_ALT_TEXT)).not.toBeInTheDocument();
109117
});
110118

111119
it('renders CMS header with default font size', () => {

‎packages/fxa-settings/src/components/CardHeader/index.tsx‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -111,14 +111,7 @@ function isDefaultService(
111111
}
112112

113113
function isCmsHeader(props: CardHeaderProps): props is CardHeaderCmsProps {
114-
return (
115-
(props as CardHeaderCmsProps).cmsLogoUrl !== undefined ||
116-
(props as CardHeaderCmsProps).cmsLogoAltText !== undefined ||
117-
(props as CardHeaderCmsProps).cmsHeadline !== undefined ||
118-
(props as CardHeaderCmsProps).cmsDescription !== undefined ||
119-
(props as CardHeaderCmsProps).cmsHeadlineFontSize !== undefined ||
120-
(props as CardHeaderCmsProps).cmsHeadlineTextColor !== undefined
121-
);
114+
return (props as CardHeaderCmsProps).cmsHeadline !== undefined;
122115
}
123116

124117
function isBasicWithCustomSubheading(
@@ -222,7 +215,9 @@ const CardHeader = (props: CardHeaderProps) => {
222215
>
223216
{cmsHeadline}
224217
</h1>
225-
<p className="card-subheader">{cmsDescription}</p>
218+
{cmsDescription && (
219+
<p className="card-subheader">{cmsDescription}</p>
220+
)}
226221
</>
227222
);
228223
}

‎packages/fxa-settings/src/models/integrations/relier-interfaces.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ export interface RelierCmsInfo {
103103
SignupPasswordlessCodePage?: PageRelierCmsInfo;
104104
SignupConfirmedSyncPage?: PageRelierCmsInfo;
105105

106-
SigninPage: PageRelierCmsInfo;
106+
SigninPage?: PageRelierCmsInfo;
107107
SigninCachedPage?: PageRelierCmsInfo;
108108
SigninTotpCodePage?: PageRelierCmsInfo;
109109
SigninPasswordlessCodePage?: PageRelierCmsInfo;

‎packages/fxa-settings/src/pages/Signin/__snapshots__/index.test.tsx.snap‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,6 @@ exports[`Signin component snapshots - CMS renders CardHeader with CMS content wh
3232
/>
3333
`;
3434

35-
exports[`Signin component snapshots - CMS renders CardHeader with CMS content when password is not needed 1`] = `
36-
<img
37-
alt="logo"
38-
class="justify-start mb-4 max-h-[40px]"
39-
src="https://cdn.accounts.firefox.com/other/firefox-browser-logo.svg"
40-
/>
41-
`;
42-
4335
exports[`Signin component snapshots - CMS renders the CMS-styled submit button 1`] = `
4436
<button
4537
class="cta-primary-cms cta-xl"

‎packages/fxa-settings/src/pages/Signin/index.stories.tsx‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,3 +239,21 @@ export const CmsCachedCachedPage = storyWithProps(
239239
},
240240
'CMS > Regular layout > Cached'
241241
);
242+
export const CmsCachedNoCachedPageConfig = storyWithProps(
243+
{
244+
sessionToken: MOCK_SESSION_TOKEN,
245+
integration: createMockSigninOAuthIntegration({
246+
cmsInfo: {
247+
...MOCK_CMS_INFO,
248+
SigninCachedPage: undefined,
249+
SigninPage: {
250+
headline: 'CMS override',
251+
description: 'just for you!',
252+
primaryButtonText: 'Click me',
253+
pageTitle: 'I am a title',
254+
},
255+
},
256+
}),
257+
},
258+
'CMS > Regular layout > Cached > No SigninCachedPage config'
259+
);

‎packages/fxa-settings/src/pages/Signin/index.test.tsx‎

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1647,7 +1647,7 @@ describe('Signin component', () => {
16471647
it('sets the shared page title from CMS', () => {
16481648
// Ensure pageTitle is not available on SigninPage, cast to type
16491649
// to allow undefined
1650-
(cmsProps.cmsInfo as RelierCmsInfo).SigninPage.pageTitle = undefined;
1650+
(cmsProps.cmsInfo as RelierCmsInfo).SigninPage!.pageTitle = undefined;
16511651
render({ integration: createMockSigninWebIntegration(cmsProps) });
16521652

16531653
expect(document.title).toBe(
@@ -1695,23 +1695,37 @@ describe('Signin component', () => {
16951695
});
16961696

16971697
it('renders CardHeader with CMS content when password is not needed', () => {
1698-
// This path: isPasswordNeededRef.current && hasPassword === false
1699-
// Renders CardHeader with different props but still uses CMS logo
1698+
const cmsProps = {
1699+
cmsInfo: {
1700+
...MOCK_CMS_INFO,
1701+
SigninCachedPage: undefined,
1702+
SigninPage: {
1703+
headline: 'CMS override',
1704+
description: 'just for you!',
1705+
primaryButtonText: 'Click me',
1706+
pageTitle: 'I am a title',
1707+
},
1708+
},
1709+
};
1710+
1711+
// No SigninCachedPage and password not needed, so CMS headline
1712+
// is not available and the heading falls back to default
17001713
render({
17011714
integration: createMockSigninWebIntegration(cmsProps),
17021715
hasPassword: false,
17031716
});
17041717

1705-
const cmsLogo = screen.getByRole('img', {
1706-
name: cmsProps.cmsInfo.shared.logoAltText,
1707-
});
1708-
expect(cmsLogo).toMatchSnapshot();
1718+
// No CMS headline from SigninCachedPage, so no CMS logo rendered
1719+
expect(
1720+
screen.queryByRole('img', {
1721+
name: cmsProps.cmsInfo.shared.logoAltText,
1722+
})
1723+
).not.toBeInTheDocument();
17091724

1710-
// When no SigninCachedPage is set, the non-password path falls back
1711-
// to SigninPage headline and description
1725+
// Falls back to the default "Sign in" text
17121726
expect(
17131727
screen.getByRole('heading', {
1714-
name: cmsProps.cmsInfo.SigninPage.headline,
1728+
name: 'Sign in',
17151729
})
17161730
).toBeInTheDocument();
17171731
});
@@ -1784,17 +1798,22 @@ describe('Signin component', () => {
17841798
).toBeInTheDocument();
17851799
});
17861800

1787-
it('falls back to SigninPage CMS config when SigninCachedPage is not set', () => {
1801+
it('falls back to headingText when SigninCachedPage is not set', () => {
17881802
render({
17891803
integration: createMockSigninWebIntegration(cmsProps),
17901804
sessionToken: MOCK_SESSION_TOKEN,
17911805
hasPassword: true,
17921806
});
17931807

1808+
// Cached users should not see SigninPage CMS headline
17941809
expect(
1795-
screen.getByRole('heading', {
1810+
screen.queryByRole('heading', {
17961811
name: cmsProps.cmsInfo.SigninPage.headline,
17971812
})
1813+
).not.toBeInTheDocument();
1814+
// Should fall back to default headingText
1815+
expect(
1816+
screen.getByRole('heading', { name: 'Sign in' })
17981817
).toBeInTheDocument();
17991818
});
18001819

‎packages/fxa-settings/src/pages/Signin/index.tsx‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -384,11 +384,13 @@ const Signin = ({
384384
const cmsInfo = integration.getCmsInfo();
385385
const cachedPageCms = cmsInfo?.SigninCachedPage;
386386
const signinPageCms = cmsInfo?.SigninPage;
387-
// Use cached page config when available and user has cached account, else fall back to SigninPage
388-
const activePageCms =
389-
!isPasswordNeeded && cachedPageCms ? cachedPageCms : signinPageCms;
387+
const activePageCms = isPasswordNeeded ? signinPageCms : cachedPageCms;
390388
const title = activePageCms?.pageTitle;
391-
const splitLayout = activePageCms?.splitLayout;
389+
// If cachedPageCms is the active page but does not have a CMS entry,
390+
// we reference the splitLayout property from the signinPageCms.
391+
const splitLayout = activePageCms
392+
? activePageCms.splitLayout
393+
: signinPageCms?.splitLayout;
392394
const additionalAccessibilityInfo =
393395
cmsInfo?.shared.additionalAccessibilityInfo;
394396

@@ -403,15 +405,15 @@ const Signin = ({
403405
}}
404406
/>
405407
)}
406-
{isPasswordNeeded && hasPassword ? (
408+
{isPasswordNeeded ? (
407409
<CardHeader
408410
headingText="Enter your password"
409411
headingAndSubheadingFtlId="signin-password-needed-header-2"
410412
{...{
411413
cmsLogoUrl: cmsInfo?.shared.logoUrl,
412414
cmsLogoAltText: cmsInfo?.shared.logoAltText,
413-
cmsHeadline: activePageCms?.headline,
414-
cmsDescription: activePageCms?.description,
415+
cmsHeadline: signinPageCms?.headline,
416+
cmsDescription: signinPageCms?.description,
415417
cmsHeadlineFontSize: cmsInfo?.shared.headlineFontSize,
416418
cmsHeadlineTextColor: cmsInfo?.shared.headlineTextColor,
417419
}}
@@ -427,8 +429,8 @@ const Signin = ({
427429
serviceName,
428430
cmsLogoUrl: cmsInfo?.shared.logoUrl,
429431
cmsLogoAltText: cmsInfo?.shared.logoAltText,
430-
cmsHeadline: activePageCms?.headline,
431-
cmsDescription: activePageCms?.description,
432+
cmsHeadline: cachedPageCms?.headline,
433+
cmsDescription: cachedPageCms?.description,
432434
cmsHeadlineFontSize: cmsInfo?.shared.headlineFontSize,
433435
cmsHeadlineTextColor: cmsInfo?.shared.headlineTextColor,
434436
}}

0 commit comments

Comments
 (0)