fix(fxa-settings): Show "Sign in" heading on cached signin page instead of "Enter your password#20189
Conversation
| // prefer the cached page config; for non-cached users, use the signin page | ||
| // config. Any of these can be undefined — CardHeader falls back to | ||
| // headingText when cmsHeadline is missing. | ||
| const activePageCms = hasCachedAccount ? cachedPageCms : signinPageCms; |
There was a problem hiding this comment.
What do you think about not using "activePageCms" like this to select which CMS page to use, and instead just rely on the conditional logic that already renders the two different CardHeaders? I'll admit, this const confused me when I first looked at this file... This reads more clear to me at least:
{isPasswordNeeded && hasPassword ? (
<CardHeader
headingText="Enter your password"
headingAndSubheadingFtlId="signin-password-needed-header-2"
{...{
cmsLogoUrl: cmsInfo?.shared.logoUrl,
cmsLogoAltText: cmsInfo?.shared.logoAltText,
cmsHeadline: signinPageCms?.headline,
cmsDescription: signinPageCms?.description,
cmsHeadlineFontSize: cmsInfo?.shared.headlineFontSize,
cmsHeadlineTextColor: cmsInfo?.shared.headlineTextColor,
}}
/>
) : (
<CardHeader
headingText="Sign in"
headingTextFtlId="signin-header"
subheadingWithDefaultServiceFtlId="signin-subheader-without-logo-default"
subheadingWithCustomServiceFtlId="signin-subheader-without-logo-with-servicename"
{...{
clientId,
serviceName,
cmsLogoUrl: cmsInfo?.shared.logoUrl,
cmsLogoAltText: cmsInfo?.shared.logoAltText,
cmsHeadline: cachedPageCms?.headline,
cmsDescription: cachedPageCms?.description,
cmsHeadlineFontSize: cmsInfo?.shared.headlineFontSize,
cmsHeadlineTextColor: cmsInfo?.shared.headlineTextColor,
}}
/>
)}
| cmsInfo: MOCK_CMS_INFO, | ||
| }), | ||
| }, | ||
| 'CMS > Regular layout > Cached > No SigninCachedPage config' |
There was a problem hiding this comment.
I think this story should probably explicitly set the page value to be undefined, otherwise if/when we add it to that mock object later this will no longer reflect the correct state.
95954eb to
1beeb01
Compare
484abbe to
24a1c99
Compare
|
|
||
| // When no SigninCachedPage is set, the non-password path falls back | ||
| // to SigninPage headline and description | ||
| // MOCK_CMS_INFO has no SigninCachedPage, so the heading falls back |
There was a problem hiding this comment.
Similar to the storybook update, this seems sorta fragile? Once we add a mock to that const this'll fail.
| const title = activePageCms?.pageTitle; | ||
| const splitLayout = activePageCms?.splitLayout; | ||
| const title = (hasCachedAccount ? cachedPageCms : signinPageCms)?.pageTitle; | ||
| const splitLayout = (hasCachedAccount ? cachedPageCms : signinPageCms) |
There was a problem hiding this comment.
Okay I can see why you did the activePage thing now. I didn't see this before, that we need to pass in the button text and need to do a check regardless:
buttonText={
(hasCachedAccount ? cachedPageCms : signinPageCms)
?.primaryButtonText
}
I reproduced the issue on main:
But in your branch currently, I see the headline issue is fixed, and there's now an issue with splitLayout (it's no showing even when it should) and the button text (showing the default instead of from the CMS). Page title probably has the same issue.
I think the check against hasCachedAccount is not the check that we want, it needs to match the check that we are doing for the headline... so Since the card header conditionally renders isPasswordNeeded && hasPassword, I think we need a new constant set like this:
const showCachedView = !(isPasswordNeeded && hasPassword);
And then you can update the CardHeader logic and base the conditional page CMS object on that.
Edit just for posterity it looks like isPasswordNeeded already checks hasPassword, so I think all we need is to check isPasswordNeeded instead of hasCachedAccount.
Because
activePageCmsselector used!isPasswordNeededas a proxy for "is cached user," so cached users who need a password gotSigninPageCMS content instead ofSigninCachedPageCardHeaderrenders empty<h1>and<p>when CMS page config is undefined but shared CMS props (logo, colors) are presentThis pull request
!hasCachedAccountguard to the heading condition inSignin/index.tsxso cached users always see "Sign in" heading (password input still renders when needed)activePageCmsto select based onhasCachedAccountinstead of!isPasswordNeededCardHeader/index.tsxto fall back toheadingTextwhencmsHeadlineis undefined, and skip rendering<p>whencmsDescriptionis missingSigninCachedPageconfigIssue
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13274
Checklist
Other Information
Looking at this a bit more, ISTM that we need a dedicated page for "cached" users. The current signin page handles both, but with cms overrides it is definately confusing.