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
New emails home screen #51452
New emails home screen #51452
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~280 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~1754 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. 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. |
client/my-sites/email/email-management/home/email-plan-view.jsx
Outdated
Show resolved
Hide resolved
client/my-sites/email/email-management/home/email-plan-mailboxes-list.jsx
Show resolved
Hide resolved
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 like how clear the logic is, and how each component are designed (except maybe the EmailPlanView
which is doing too many things). That's a lot of code so I left several comments but most of them are minor.
Not for this pull request but we should probably make sure that all pages of the domain and email management sections have the same width, as right now it's not consistent. We could also order the lists of domains alphabetically on the new Email Home
page.
client/my-sites/email/email-management/home/email-plan-view.jsx
Outdated
Show resolved
Hide resolved
client/my-sites/email/email-management/home/email-list-active.jsx
Outdated
Show resolved
Hide resolved
} | ||
|
||
return true; | ||
} |
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.
Aren't we performing eligibility checks server-side now? @daledupreez or @olaseni may be able to provide more details.
client/my-sites/email/email-management/home/email-plan-mailboxes-list.jsx
Outdated
Show resolved
Hide resolved
client/my-sites/email/email-management/home/email-plan-view.jsx
Outdated
Show resolved
Hide resolved
</React.Fragment> | ||
); | ||
} | ||
} |
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 would recommend refactoring this component. At first I thought it would handle all types of email accounts. Then I started reading it, and found that it actually contains lots of logic that is specific to Titan. And finally, I've realized that it's indeed for G Suite and email forwards when I reached the part where we connect it to the Redux state tree. We should really separate responsibilities, extract the code that retrieves the list of Titan mailboxes in a Query component, moves the normalization logic in another component ...
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.
The code is in there temporary - I just didn't want to add all the boilerplate code in one single PR as that would make the PR even bigger. This is actually on my list of next tasks - "Move the Titan mailboxes loading into global 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.
Gotcha. I've mentioned it in the server patch but I believe we could simplify this code a lot if data normalization happened on the server, and if we used one API endpoint to retrieve all the email accounts (may they be for Titan or Google Workspace).
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.
Actually there is a stub for this called QueryEmailAccounts
and related redux stuff in client/state/email-accounts
but it would require a lot more changed to actually make this work so I decided on making this PR work instead and leave the refactoring for later. It would definitely make the code simpler but it's just too big to include in this PR.
8924550
to
e6b2911
Compare
@saygunnyc, when you get online today, I would really appreciate your eyes on the changes here. We're not all the way there yet in terms of implementation, but this gets the bulk of the infrastructure into place. (There are plenty of TODOs to take on!) In particular, there is one smaller item where I'd like you to weigh in, and then a much broader one as well. Vertical spacingThe first area that would be good to align on is vertical spacing. The designs have all the cards in lists with a height of 80px. In the main home page, I think that makes sense, as we have a lot going on in each row, including buttons and/or logos, so the space is likely necessary. On the domain/account-specific page, the default spacing for cards has the height way less at around 56px, as all the rows only contain text. In this iteration, I've increased the padding to 24px above and below the text, but this only brings the height for these rows to 72px. LMK if you think we should increase the padding to 28px either side to keep the element spacing the same vs the internal spacing. Staging the remaining workWhile our goal is clearly to ship all the improvements to users, I also don't want to defer getting all the improvements into place before we launch the new pages. From my side, I think the following items are absolute must-haves. Please weigh in with any not on my list that you think we need for launch. Proposed must-haves
Proposed non-blocking items
|
Hey Dale,
Yes, let's have all be consistent. Same height for both email products and mailboxes. Mailboxes will also have a lot going on soon with the addition of the overflow menu, nudges, and CTAs, so I'd say it's better to do the change now rather than later.
At a high level, I would say we shouldn't be removing any functionalities or links that users can reach today. An example of that would be having links to Gmail, Slides, etc. on Google Workspace as well as Inbox, Calendar, etc. on Titan. I'm open to staging what makes the most sense for the rest but in a frame where we're not creating UX issues or taking away something we provided earlier to the user. One another question,
Do we have a case like this? If yes, then we should direct the user to take action. What's the required action for the user in this case? How does the empty mailbox list look like? |
* extract the email type icon * fixed the active email list styling * fixed the active email mailbox count string
9bd4946
to
66c37aa
Compare
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've tested it, and everything seems to work fine - apart from the limitations already mentioned in the description of this pull request. I've found the following issues though:
- We don't display any
Finish Setup
notice for Google Workspace and G Suite - We don't display any
Finish Setup
button for Google Workspace and G Suite - We don't display any
You have n unused mailbox
message for Titan - The
Back
button always redirects to the generalEmail
page even if coming from theDomain Settings
page
All of this, plus the suggestions I've made in some comments, can of course be addressed in future iterations. It would be good to address issue #1 and #2 asap though.
|
||
export function isEmailUserAdmin( emailUser ) { | ||
return EMAIL_USER_ROLE_ADMIN === emailUser?.role; | ||
} |
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'm concerned that we're introducing yet another name for something we usually call a user
for G Suite, and mailbox
for Titan. We should really agree on a term.
@@ -759,6 +759,18 @@ Undocumented.prototype.validateGoogleAppsContactInformation = function ( | |||
return result.then?.( camelCaseKeys ); | |||
}; | |||
|
|||
Undocumented.prototype.getEmailAccountsForSiteAndDomain = function ( siteId, domain, fn ) { |
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.
Shouldn't the name reflect the fact that the list of accounts retrieved will also contain the list of mailboxes? What will happen when we start using this API endpoint to only retrieve accounts (and not mailboxes)?
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.
My thinking here is that I want this to be replaced/updated pretty soon.
In terms of naming, does it make sense to call this getMailboxesForSiteAndDomain()
? Or should we call it something else?
return this.renderContentWithHeader( | ||
<> | ||
<SectionHeader className="email-home__section-placeholder is-placeholder" /> | ||
<Card className="email-home__content-placeholder is-placeholder" /> |
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 really need to define the email-home__section-placeholder
and email-home__content-placeholder
classes? There are never used anywhere.
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 don't think I added these specific classes, but this is a "feature" of the linter -- all initial class names need to include an acceptable prefix.
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 understand that the linter may complain if a class name doesn't have the right prefix. Do you mean it also added those class names itself (assuming you used it with the --fix
argument)?
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.
No, the issue is that if you try to add just a different classname, like className="is-placeholder"
, the linter won't accept that. One workaround is to then use className="some_compliant_prefix__something is-placeholder"
. I am not saying that it's a good approach, but it's one that is linter-compliant. :-/
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.
Got it, thanks for the clarification. There is not much we can do indeed 😞.
client/my-sites/email/email-management/home/email-plan-subscription.jsx
Outdated
Show resolved
Hide resolved
return ''; | ||
} | ||
|
||
export function resolveEmailPlanStatus( domain ) { |
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'm not a big fan of that function name as it doesn't provide any clue regarding the type of data returned. Maybe it does too much? Looking at how it is used, maybe we could have two distinct functions - one that returns if the status is ok or not, and another one that renders this status:
export function renderEmailPlanStatus( domain ) {
let icon = 'check_circle';
let text = translate( 'Active' );
if ( hasPendingGSuiteUsers( domain ) ) {
icon = 'info';
text = translate( 'Action required' );
}
return (
<span className="email-plan__status">
<MaterialIcon icon={ icon } /> { text }
</span>
);
}
width: 150px; | ||
height: 28px; | ||
} | ||
} |
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.
All of this should most probably be moved to a new stylesheet in the home
subfolder.
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.
Agreed.
return translate( 'Email settings' ); | ||
} | ||
|
||
return translate( 'Email forwarding settings' ); |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 32 times:
translate( 'Email Forwarding' )
ES Score: 6
const managePurchaseUrl = getManagePurchaseUrlFor( selectedSite.slug, purchase.id ); | ||
return ( | ||
<VerticalNavItem path={ managePurchaseUrl }> | ||
{ translate( 'Billing and payment settings' ) } |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 18 times:
translate( 'Update your billing and payment settings' )
ES Score: 7
<div className="email-plan__actions"> | ||
<VerticalNav> | ||
<VerticalNavItem { ...addMailboxProps }> | ||
{ translate( 'Add new mailbox' ) } |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 17 times:
translate( 'Add New Mailboxes' )
ES Score: 8
How are we planning to resolve these cases if not within the interface? Can you expand this a little, so I understand better. |
I'm not sure to understand your question @saygunnyc 🙂. I personally think those issues should be addressed prior to launching this new
Sure. If I'm on the new |
Yes, all make sense to me now! Thank you, @stephanethomas for clarifying! |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/5748853 Thank you @hambai for including a screenshot in the description! This is really helpful for our translators. |
|
||
state = { | ||
isLoadingEmailAccounts: false, | ||
errorLoadingEmailAccounts: false, |
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.
Seems like we set this state property but we never use it. Maybe it can be removed?
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.
Thanks Marin! I will remove it in #52815.
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.
Thanks, @stephanethomas 🙌
Translation for this Pull Request has now been finished. |
This is an initial PR to create the new/updated email management screens. @hambai built most of the infrastructure, but we delayed some work to wait for updated designs. The code is heavily based on #50790 but in the end it was easier to start clean and manually move stuff from the old PR.
The PR has been updated to fold in the updated designs in pcDL0W-al-p2, though we still have a ways to go before we get all the elements in.
Changes proposed in this Pull Request
Screenshots
The email list:
Email forwards:
Note that the list of email forwards also needs D60279-code to land.
WordPress.com Email:
G Suite:
Known Issues and TODOs
This PR is mostly intended to land the core functionality behind the
email/centralized-home
feature flag so we can iterate with much smaller patches. It also means we can get some clear feedback from design on how to prioritise remaining tasks.We still have the following items outstanding:
Testing instructions
/email?flags=email/centralized-home
on the calypso.live branch/email/siteSlug.tld
)calypso.localhost
, the WordPress.com Email link should not be internal, as we show the iframed control panel in that case.In general, please keep an eye out for any weirdness/bugs. We'll likely fix them in follow-up PRs, but it would be good to catch anything like this. (I already tripped over an issue with the
VerticalNavItem
placeholder display that I fixed in #52028!)