-
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
Domains: Implement multi-target email forwards #98837
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 (~193 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~153611 bytes removed 📉 [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 (~12256 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. |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/17215663 Some locales (Hebrew, Japanese, Spanish) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Hi @alshakero, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:
Thank you in advance! |
@crisbusquets Hi there! I built what I thought made sense while design is being done. Would be great if you can review what I came up with 🙏🏼 You can use the Live link, then go to /domains/manage/all/email/alshaker.me/alshaker.me (I added you to the site) and look at it. Sadly you won't be able to make changes unless you do some sandboxing and such. |
|
||
/** | ||
* @param newEmailForward a string representing a new email forward | ||
* @returns { boolean } If the email forward is has more than the maximum number of destinations. |
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.
Probably need to rephrase this
@alshakero - I was testing the validation on multiple fields simultaneously and noticed this behavior where the Add button isn't enable. I don't think the average person will do this, but I just wanted to point it out. Also, erasing an email removes all the valid and invalid emails below it. CleanShot.2025-01-28.at.18.50.34.mp4 |
In the code I see that it is a current limitation that we have, worth it to double check probably. I'm sure @alshakero already did that. |
</Heading> | ||
<Text> | ||
{ translate( | ||
"This will remove it from our records and if it's not used in another forward, it will require reverficiaton if added again." |
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.
Spelling for reverficiaton
to reverification
mailbox.warnings?.length | ||
? [ | ||
{ | ||
title: 'Resend', |
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.
Let's add translations for the title property. Resend
and Remove
Note to self: in a follow up PR, I'll move the request to the form itself. Currently, if there are issues with the data, they appear after submission and the user loses thier data. |
client/my-sites/email/email-forwarding/verification-notices/index.tsx
Outdated
Show resolved
Hide resolved
@alshakero - The mobile view needs some love: |
Mobile support is done. Waiting for @auareyou's approval. |
@Automattic/vertex mobile version is good to go. |
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.
Everything is working well with the associated backend PR.
We still need to update the ActionMenu
item title translations.
Minor suggestion for mobile view: Add wrapping for the email addresses
Translation for this Pull Request has now been finished. |
Requires https://github.a8c.com/Automattic/wpcom/pull/171065
Related to #98801
Proposed Changes
This implements the ability to add multiple email forwards for a single mailbox. It allows up to 5 addresses per mailbox.
Why are these changes being made?
See: p58i-jdB-p2
Testing Instructions
Testing under Upgrades > Emails
Validation
Deletion
Verification
Testing under /sites > Domains > Email (Tab)
Repeat all the above. The only difference, is that "Add mailbox" is on top now.
Testing under Hosting settings
Repeat all the above. The only difference, is that "Add mailbox" is on top now.
URL: /overview/site-domain/email/DOMAIN/DOMAIN
Regression testing (important)