-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update email verification items - remove old notices, add banner, update copies and success messages. #94683
Update email verification items - remove old notices, add banner, update copies and success messages. #94683
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~83 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~83305 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~5042 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
…age with that form
…ue with input jumping to pending value if you type in old email value.
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Thanks for looking @javierarce and nice finds! I updated the items you mentioned. The button should show a cursor on hover now, the dialog should close on overlay click, and scroll/focus functionality should work when using the modal from a separate page. |
// Check for query param used to indicate a need to scroll to the input, this is added when | ||
// redirecting to this page for the purpose of email change. | ||
const urlQueryParams = new URL( window.location.href )?.searchParams; | ||
const focusEmail = urlQueryParams.get( 'focusEmail' ); | ||
if ( focusEmail ) { | ||
scrollAndFocus(); | ||
window.history.replaceState( {}, '', removeQueryArgs( window.location.href, 'focusEmail' ) ); | ||
} | ||
// Listen for an event signalling to focus and scroll to the email field or button. This | ||
// happens when we are already on a page with the input and need to trigger the | ||
// functionality. | ||
emailFormEventEmitter.addEventListener( 'highlightInput', scrollAndFocus ); |
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.
Instead of using a query param for cases coming from another page, and an event emitter for cases on triggering this on the same page, I think the other option is moving this to Redux but I am conflicted. In both cases this is more of a signal/event to trigger functionality and not related to any state to manage, so Redux seems a little weird but we could probably manage it with a true/false toggle. But that seems like potential to get out-of-sync in some way (?), and generally it feels like extra boilerplate code and overkill for the purpose.
Id be curious thoughts of other developers reviewing.
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.
I think the query param implementation makes sense. But does it need the same isEmailChangePending
conditional check?
Also, I think the scroll only works the first time in Firefox. 🤔
Screen.Recording.2024-09-25.at.4.09.44.PM.mov
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.
But does it need the same isEmailChangePending conditional check?
Yeah. If the change is pending the input is disabled, so instead we focus the button that cancels the pending change and unlocks the input. So either way that check is relevant to figure out what element to focus.
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.
Also, I think the scroll only works the first time in Firefox. 🤔
Ah interesting! I don't think its just firefox.
So if you open the modal on /me/account
and click "change" scroll works every time. But if you go to /me
, open the modal, and click "change", the scroll/focus it only works if you haven't loaded /me/account yet.
I didn't think this would be the case but for whatever reason it seems like the email form component on /me/account doesn't dismount when you go back to /me. So the hook checking for the query param on mount to scroll to the input isn't firing since the component has been mounted and none of its dependencies have changed.
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.
Ok not component mounting like i suspected, its the component loading before the window url is set in this case.
this should now be resolved with df256f2.
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.
This is testing well for me! I left a couple of comments.
Also, I have some feedback on the modal, but this could be suitable for a new issue.
I know this is existing copy, but it’s a little confusing. It says to "open the email and click the blue button", but the button in the modal is blue, could it mean that button? 😅 Additionally, if this is a second email change, the verification email is text-based, and there is no button in that email. Finally, the second paragraph in the modal has a smaller font size.
background: var(--color-surface); | ||
border-radius: 8px; /* stylelint-disable-line scales/radii */ | ||
box-shadow: 0 0 17.4px 0 rgba(0, 0, 0, 0.05); | ||
overflow-y: auto; | ||
overflow-x: hidden; |
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.
// Check for query param used to indicate a need to scroll to the input, this is added when | ||
// redirecting to this page for the purpose of email change. | ||
const urlQueryParams = new URL( window.location.href )?.searchParams; | ||
const focusEmail = urlQueryParams.get( 'focusEmail' ); | ||
if ( focusEmail ) { | ||
scrollAndFocus(); | ||
window.history.replaceState( {}, '', removeQueryArgs( window.location.href, 'focusEmail' ) ); | ||
} | ||
// Listen for an event signalling to focus and scroll to the email field or button. This | ||
// happens when we are already on a page with the input and need to trigger the | ||
// functionality. | ||
emailFormEventEmitter.addEventListener( 'highlightInput', scrollAndFocus ); |
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.
I think the query param implementation makes sense. But does it need the same isEmailChangePending
conditional check?
Also, I think the scroll only works the first time in Firefox. 🤔
Screen.Recording.2024-09-25.at.4.09.44.PM.mov
Note the modal itself has been updated separately in #94629 and waiting translations. |
Oh! I missed that. Thanks! 😄 |
… previously loaded
Translation for this Pull Request has now been finished. |
@DustyReagan - This has been updated to resolve the issues you found, and rebased to include the changes to the dialog modal that hopefully fit a bit better now. 😄 @javierarce Question for you. Above Dusty found a regression with our layout frame width in larger viewports. I have fixed the regression but it leaves a question for the Banner width. I have currently kept the banner width the same max-width as the content inside the frame: I assume the above may be ideal. The other option is to let the banner expand with the edges of the frame itself which could feel a bit odd at wider widths: Any thoughts there? |
In this case, I think doing that (keeping the banner width the same max-width as the content) is the right solution. |
…nner in some circumstances
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.
Tests well and the changes look good to me! Nice work! 👍
It seems the string freeze check froze at 86% but was actually 100% done. Removing and re-adding the label seemed to update it. 🤷♀️ |
Related to # #93895 and #94247
Proposed Changes
Removes the email not verified notice and email pending change notice that were previously underneath the input and moves these concerns into the input's explanation text, a new banner, and new success message on successful email change. The various forms of the explanation text are shown immediately below:
Adds a new email verification banner at the top of /me and /me/account that opens the email verification dialog when clicked.
The email verification dialog has been updated to close on route change, and to send an event to signal focus/scroll behavior on the email input if on /me/account. It has also been updated to allow resending an email for pending email change, as it was previously only set up for verification email.
updated to close on route change
- previously this dialog was not shown /me/account, so clicking the link to /me/account would close the dialog since its parent controller was no longer rendered. Now that we have the new verification banner controlling the dialog which is also shown on /me/account, the parent controller does not dismount and close the dialog. Therefore we ensure it closes on route change.Adds a SuccessNotice when the email is updated, mentioning an email was sent to the new address.
This has been added on both /me/account as well as /me/security/account-email
On /me/account, the success notice copy is conditional to the changes made (other settings only, email only, or both email and other settings).
Adds a success notice when the user is redirected to /me/account after confirming a pending email change. This contains the previous information about updating domain information if necessary and a the same link to updating domains contact info that was previously in the notice.
/settings/account
to/me/account
to include the?new_email_result=1
param if it exists, and other middleware will check for this param, remove it, and add the corresponding notice.POTENTIAL FOLLOW-UPs
isCritical
orisWarning
type case. Currently it is set to inherit interface colors (or maybe jetpack colors), and we override styles for our specific use to get the red. If this is intended to be used for more warnings in the future, we should move those styles into the banner component and allow them to be enabled with a prop.BEFORE
AFTER
As shown above in description items.
Why are these changes being made?
Testing Instructions
*/settings/account?new_email_result=1
. Verify this redirects to /me/account, they query param is gone, and there is a success notice with a link to domains.*/settings/account?new_email_result=1
.Pre-merge Checklist