-
Notifications
You must be signed in to change notification settings - Fork 332
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
upcoming: [M3-7476] - Add Verification Banner for Child Accounts #10085
Conversation
Waiting on #10083 Edit: β |
} | ||
}, 100); | ||
} | ||
}, [phoneNumberRef, securityQuestionRef, location.state]); |
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.
Focus and scroll to a specific input field based on the values provided in the location state.
packages/manager/src/features/GlobalNotifications/GlobalNotifications.tsx
Outdated
Show resolved
Hide resolved
β¦cations.tsx Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>
Coverage Report: β
|
@abailly-akamai @bnussman-akamai all set. |
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.
Tested with MSW - Behavior and UI look great, saw the banner and was scrolled down to the proper field upon clicking the banner button β
I left non-blocking comment and question to potentially improve the code
currentTargetRef.scrollIntoView(); | ||
currentTargetRef.setAttribute('data-scrolled', 'true'); | ||
} | ||
}, 100); |
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.
Can we use a MutationObserver
here rather than a setTimeout
? I think it's better to avoid those for this kind of case
@@ -68,7 +111,7 @@ export const AuthenticationSettings = () => { | |||
Learn more about security options. | |||
</Link> | |||
</StyledMainCopy> | |||
<PhoneVerification /> | |||
<PhoneVerification phoneNumberRef={phoneNumberRef} /> | |||
<Divider spacingBottom={16} spacingTop={22} /> | |||
<Typography variant="h3">SMS Messaging</Typography> | |||
<SMSMessaging /> |
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.
Do we need to invalidate useQueryClient
or useSecurityQuestions
at any point after submitting the security questions?
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.
useMutateSecurityQuestions
should handle the cache updates already
@@ -68,7 +111,7 @@ export const AuthenticationSettings = () => { | |||
Learn more about security options. | |||
</Link> | |||
</StyledMainCopy> | |||
<PhoneVerification /> | |||
<PhoneVerification phoneNumberRef={phoneNumberRef} /> | |||
<Divider spacingBottom={16} spacingTop={22} /> | |||
<Typography variant="h3">SMS Messaging</Typography> | |||
<SMSMessaging /> |
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.
useMutateSecurityQuestions
should handle the cache updates already
Description π
This adds a verification banner to be displayed for child users lacking security questions and a phone number, So that we can prompt them to enhance the security of their accounts. The banner will display on all pages for child users.
Changes π
AuthenticationSettings.tsx
contains logic to focus either the security questions or phone number depending on what's left to verify.Preview π·
How to test π§ͺ
Prerequisites
Reproduction steps
verified_phone_number
isnull
in profile.tsVerification steps
/profile/auth
/profile/auth
As an Author I have considered π€
Check all that apply