Emails: Mailbox list and add/remove button design changes #55529

runofthemillgeek · 2021-08-16T12:43:50Z

Changes proposed in this Pull Request

Related to #55183

This PR introduces design changes as per Figma mockups (see P2 for Figma link and details) for Professional Email's mailbox list form components. Specifically:

Each additional mailbox has a numbered header
Remove mailbox button has been moved next to the Add mailbox button in both Desktop and Mobile layouts and is no longer an icon button

Testing instructions

Visit http://calypso.localhost:3000/email/:siteSlug
Click Add email
Click Add another mailbox
Test in desktop/mobile layouts

Screenshots

Click/tap on this section to see screenshots

Mobile (/email/:domain/purchase/:siteSlug)

Before	After

Desktop (/email/:domain/purchase/:siteSlug)

Before	After

Mobile (/email/:domain/titan/new/:siteSlug)

Before	After

Desktop (/email/:domain/titan/new/:siteSlug)

Before	After

CC @daledupreez

a8ci18n · 2021-08-16T12:49:53Z

client/my-sites/email/titan-new-mailbox-list/index.jsx

+						{ index > 0 && (
+							<Button onClick={ onMailboxRemove( mailboxes, mailbox.uuid ) }>
+								<Gridicon icon="trash" />
+								<span>{ translate( 'Remove this mailbox' ) }</span>


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 18 times:
translate( 'Remove mailbox' ) ES Score: 6

stephanethomas · 2021-08-16T14:40:46Z

client/my-sites/email/titan-new-mailbox/index.jsx

 				</div>
 				<FormFieldset>
 					<FormLabel>
 						{ showLabels && translate( 'Email address' ) }
 						<FormTextInputWithAffixes
-							placeholder={ translate( 'Email' ) }
+							placeholder={ translate( 'Email address' ) }


Just an idea but instead of repeating the label just above, we could actually provide the user with a suggestion: e.g. contact. We already do that for email forwards:

Not related to this PR, but just a small note on copies is that would be great to start each with uppercase and then use lowercase, similar to Professional Email detail pages.
"Emails sent to"
"Will be forwarded to"
"Your existing email address"

And separately I think we should also tweak the CTA copy to be "Add new email forwarding" instead of "Add new email address" for clarity.

Being looked at in Automattic#55529

* Use EmailProvidersComparison for upsell page * Hide email forwarding and header cards * Add header title and description prop * Extract out titan mailbox components to fix styles * Add skip button to header cake * Change email placeholder text * Change style and layout for mailbox add/remove * Fix ordering * Add position="left" to main HeaderCakeBack * Remover previous trash icon button * Remove unused button classname * Change button prop name to iconPosition * Handle back button click * Use actionButton prop for skip * Add cartDomainName prop * skip checks if cartDomainName is present * Fix import ordering * Remove unnecessary fragment * Pass title and description props as strings * Remove "from" to avoid re-translation * Sort and clean-up propTypes definitions * Rename is* props to hide* * Remove promoHeaderDescription prop * Rename isSkippable to showSkipButton * Add domainName to propTypes * Add back style.scss import in titan-add-mailboxes * Don't pass domains prop if `domain` prop is absent * Add hasCartDomain dervied prop * Revert titan mailbox submit button text * Undo mailbox form UX changes - being looked at in #55529 * Pass empty array to getItemsForCart if domain is undefined * Fix variable ordering

daledupreez

Overall, this is really close, but there are some CSS fixes that are still needed.

daledupreez · 2021-08-20T12:48:26Z

client/my-sites/email/titan-new-mailbox-list/index.jsx

+							tagName="h3"
+							size={ 20 }
+						>
+							Mailbox { String( index + 1 ).padStart( 2, '0' ) }


This string should be wrapped in a translate() call with the mailbox number substituted in.

I am also thinking that the 0-padding is unusual -- extremely few users are going to buy more than 9 mailboxes at a time. @saygunnyc, please chime in if you think this is something you think we should keep as 02, 03, etc. I would prefer just 2/3/etc.

Missed adding that, thanks for catch. Have added transalte() around this.

On a side note, I agree with omitting the 0 as well.

daledupreez · 2021-08-20T14:14:29Z

client/my-sites/email/titan-new-mailbox-list/style.scss

@@ -23,3 +23,13 @@
 		}


I think we also need to add some left/right padding to the buttons for non-mobile displays when we're outside the email comparison page. The screenshot below was on Chrome at desktop width looking at the add mailbox page.

For this use case, I think it might make sense to use something like the following as the default styling:

@include break-mobile { .button + .button { /* this could also be a :not:first-child selector */ margin-left: 1.5em; } }

We can then override that for the email comparison page, where we're currently specifying justify-content: space-between for the wrapping/container element set to flex.

We also have a similar issue for that same page in mobile, where we're not switching the Cancel and Continue buttons to flex-direction: column;:

I took a different direction for this with 15ca39a by adding a wrapper for the children and giving it the same styles as the existing action buttons. This seems to work well on /titan/new, /purchase and the new /email routes which I've checked so far. I'll expand further on this, think a bit more next week.

Apologize for the late response over this PR, was just able to get here.
Sharing the designs here for visibility and for parity in design & development: CTA copy, padding between buttons, button alignment, and treatment.

…mailbox-design-changes

runofthemillgeek · 2021-08-20T19:54:11Z

Few things that popped up while I was working on Dale's comments:

The remove button is on top in mobile layout according to Figma:

This was not the case in this PR until I added f39a397 where I used column-reverse to make it appear so. But a11y wise, this doesn't seem to be a good solution. Is that a concern at present?

@daledupreez
I missed the fact that the design for multiple mailboxes doesn't specify the option add mailboxes in-between, which is how I've implemented it at the moment. 🙈 I'll make changes to simplify this.

@daledupreez @saygunnyc
The remove button in Figma uses an outline variant of the trash icon. This doesn't seem to be available in the icon library.

@daledupreez @saygunnyc
Since @daledupreez mentioned the pages that fall outside <EmailProvidersComparison> and how this change would impact them, I figured it'd probably be a good idea to get @saygunnyc's perspective here and maybe consider adding design files for these. I couldn't find changes for this particular page/section in the product component designs shared. Here's one such page (before/after):

https://wordpress.com/email/:domain/titan/new/:siteSlug

Before After (this PR)

daledupreez

Overall, I think this is looking really good - I think my only blocking comment is around the Mailbox N translation.

Beyond that, I think we're going to need @saygunnyc to explore some of the UX pieces, may be a bit difficult due to us not getting a live branch/build due to security limitations.

@saygunnyc, could you propose an approach that would give you a feel for the UX across the various components? Might it make sense for you and I to pair on this?

Sangeeth, I'll also address your questions in a dedicated comment.

daledupreez · 2021-08-23T10:33:48Z

client/my-sites/email/titan-new-mailbox-list/index.jsx

+							{ translate( 'Mailbox %(index)s', {
+								args: { index: String( index + 1 ).padStart( 2, '0' ) },
+							} ) }


Thanks for adding the translate() wrapper!

Could you also add a comment for translators? It may also be clearer to call this %(position)s for the purposes of translation.

Suggested change

{ translate( 'Mailbox %(index)s', {

args: { index: String( index + 1 ).padStart( 2, '0' ) },

} ) }

{ translate( 'Mailbox %(position)s', {

args: { position: String( index + 1 ).padStart( 2, '0' ) },

comment: '%(position)s is the position of the mailbox in a list, e.g. Mailbox 1, Mailbox 2, etc',

} ) }

Also, let's drop the padding to start with -- unless we hear strong objections from @saygunnyc, I'd prefer to make this mainstream. (We can easily change it to match the design if and when he weighs in.)

Thanks for the feedback. Have incorporated these suggestions.

daledupreez · 2021-08-23T10:36:06Z

client/my-sites/email/titan-new-mailbox-list/index.jsx

@@ -110,7 +110,7 @@ const TitanNewMailboxList = ( {
 				</React.Fragment>
 			) ) }

-			{ children }
+			<div className="titan-new-mailbox-list__parent-actions">{ children }</div>


Calling this parent-actions feels weird to me, but I can't put my finger on why!

Might supplied-actions be a bit more self-descriptive?

Yea, didn't feel 💯 sure about naming it like that. I think supplied-actions sound better.

daledupreez · 2021-08-23T10:37:14Z

client/my-sites/email/titan-new-mailbox-list/style.scss

 		display: flex;
-		flex-direction: column;
+		flex-direction: column-reverse;
+		justify-content: space-between;

 		@include break-mobile {
 			flex-direction: row;


Should we not revert/override the justify-content directive here as well?

Maybe the way I've written this was confusing. The Figma design under Domain Purchase Flow i1 > Revisit section shows:

So for the main Continue/Cancel button section, in desktop mode, it seems like the default should be justify-content: start. For the Add mailbox/Remove mailbox section, we needed space-between.

At mobile configuration (i.e column), justify-content can remain the default value for both sections and it doesn't seem to matter.

I refactored this part slightly to reflect this better. Let me know if you still think something's amiss. @daledupreez

daledupreez · 2021-08-23T10:38:51Z

client/my-sites/email/titan-new-mailbox-list/style.scss


 			@include break-mobile {
-				margin-bottom: 0;
+				margin-top: 0;
+				margin-right: 1.5em;


I worry about this triggering weird overflow conditions at specific sizes. I'll test this out to see how it behaves in practice.

Nope, looks OK to me. The one weird thing is that the main buttons for save/continue are not using justify-content until we're hitting mobile widths.

daledupreez · 2021-08-23T14:04:53Z

Few things that popped up while I was working on Dale's comments:

The remove button is on top in mobile layout according to Figma:

This was not the case in this PR until I added f39a397 where I used column-reverse to make it appear so. But a11y wise, this doesn't seem to be a good solution. Is that a concern at present?

@saygunnyc, do you have a strong preference for this display order? I don't -- I'd prefer to prioritise accessibility. That said, this should only occur on mobile devices, I don't know how many accessibility tools exist in that context.

I missed the fact that the design for multiple mailboxes doesn't specify the option add mailboxes in-between, which is how I've implemented it at the moment. 🙈 I'll make changes to simplify this.

Let us know if this is still something you plan to tackle.

The remove button in Figma uses an outline variant of the trash icon. This doesn't seem to be available in the icon library.

I think we may be able to use the Material icons for this. @saygunnyc, let us know if this should be taken on here or in a follow-up PR.

runofthemillgeek · 2021-08-23T16:55:46Z

Let us know if this is still something you plan to tackle.

I've addressed this in 9429660. But I do have one query after making this change. In the following case where there are two additional mailboxes:

Should the additional mailboxes (excluding the last one) have the remove button on the left (present implementation) or the right? The Figma file doesn't seem to have this case.

CC @saygunnyc @daledupreez

daledupreez · 2021-08-25T19:37:14Z

Apologies for the delay, @sangeeth96!

Regarding your question about how we should display the Remove button when it's the only button, I prefer it being on the right and in the same location regardless of whether the "add mailbox" button is visible. Having the location change depending on other state feels really confusing and unexpected to me.

I also played with implementing the above and think the simplest approach would be to declare a class on the remove button (something like className="titan-new-mailbox-list__action-remove") and then adding a CSS selector like:

	.titan-new-mailbox-list__actions {
		@include break-mobile {
			justify-content: space-between;
+
+			.titan-new-mailbox-list__action-remove {
+				margin-left: auto;
+			}
		}
	}

I also thought more about the trash icon, and the icon can be added using <MaterialIcon icon="delete" />, but we'll need to tackle some deeper CSS issues with the material icons and our default button styling, so I think we should defer that for now.

saygunnyc · 2021-08-25T21:38:32Z

@sangeeth96 great progress here already!! 🍾 Apologies for just getting here. @daledupreez that would be great to briefly connect about this tomorrow, if you happen to have the time.

A few notes in the meanwhile,

Let's make sure we use the same iconography for removal, both here and over the overflow menu for a better UX. The intention was to use the Material icon in both experiences.
It is intentional to have the removal CTA show up first and then have "Add another mailbox". Removal is more relevant to the fields above and adding new will open up a new form below.
Overall, that would be great to follow the direction on Figma unless we have any concerns.

daledupreez · 2021-08-26T10:24:28Z

@saygunnyc, feel free to ping me when you get in today.

I do have some responses to your comments:

Let's make sure we use the same iconography for removal, both here and over the overflow menu for a better UX. The intention was to use the Material icon in both experiences.

I think we should take this on in a follow-up PR, for two reasons:

The default styling for Material icons within buttons is fairly poor, so this will be a big scope jump if we might impact all Material icons within buttons -- it's also something one of the Automattic team members ought to take on.
The Google Workspace and Professional Email forms are both using the same icon -- I'd rather change both of them together than have a mismatch on the same screen (both for the upsell and the standard email comparison page).

It is intentional to have the removal CTA show up first and then have "Add another mailbox". Removal is more relevant to the fields above and adding new will open up a new form below.

We have this in place, so we're good to go!

…mailbox-design-changes

runofthemillgeek · 2021-08-26T14:43:45Z

@daledupreez have updated the screenshots with changes from latest commit (bb84b0b)

cc @saygunnyc

daledupreez

This is LGTM! I've just connected with Saygun and we have two follow-up PRs that we'll take on:

Fix the icons in the buttons
Move the "Add another mailbox button" back out of the main list, and left-align the "Remove this mailbox" buttons

runofthemillgeek · 2021-08-26T15:09:08Z

Thanks a lot @daledupreez @saygunnyc taking the time to iron this out.

Move the "Add another mailbox button" back out of the main list

If I understand correctly, this will make that button fall on the same line as "Add professional email" CTA?

, and left-align the "Remove this mailbox" buttons

@saygunnyc would love it if I could get updated Figma designs for this with "two additional mailboxes" scenario which should cover those use cases as well.

@daledupreez @saygunnyc hope you folks also managed to discuss the additional skip button and Figma designs for those as well.

saygunnyc · 2021-08-26T15:25:08Z

Hey @sangeeth96 !
For sure, I've quickly added the button placement modifications to our existing Figma file.
The goal here is to hierarchically place the buttons in the right context, which the previous design was actually slightly missing on.

Single Mailbox

Multiple Mailboxes

Within the Figma file
You'll see the new Frame called "Button Placement Modifications" over the page called "Epic01 - Onboarding : Product Components i1"

Hope this is helpful!

a8ci18n · 2021-08-26T19:16:14Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/6486348

Thank you @sangeeth96 for including a screenshot in the description! This is really helpful for our translators.

a8ci18n · 2021-09-05T15:28:43Z

Translation for this Pull Request has now been finished.

Sangeeth Sudheer added 2 commits August 16, 2021 13:29

Update design of titan new mailbox form

656040a

Increase card heading margin and line height

ff18802

runofthemillgeek requested a review from a team as a code owner August 16, 2021 12:43

runofthemillgeek mentioned this pull request Aug 16, 2021

Domains: Add titan email upsell page contents #55183

Merged

3 tasks

github-actions bot added the OSS Citizen label Aug 16, 2021

a8ci18n reviewed Aug 16, 2021

View reviewed changes

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 16, 2021

stephanethomas reviewed Aug 16, 2021

View reviewed changes

runofthemillgeek pushed a commit to talk-to/wp-calypso that referenced this pull request Aug 16, 2021

Undo mailbox form UX changes

b397801

Being looked at in Automattic#55529

bluefuton added the Professional Email Issues with Professional Email powered by Titan label Aug 16, 2021

daledupreez reviewed Aug 20, 2021

View reviewed changes

Sangeeth Sudheer added 5 commits August 20, 2021 22:15

Merge remote-tracking branch 'upstream/trunk' into update/add-remove-…

08766af

…mailbox-design-changes

Use translate() for mailbox header text

ee3ecbe

Add spacing between add and remove buttons

e294f81

Place remove button on top visually in mobile

f39a397

Add wrapper div for parent action buttons

15ca39a

daledupreez reviewed Aug 23, 2021

View reviewed changes

Sangeeth Sudheer added 5 commits August 23, 2021 20:24

Remove mailbox position text zero pad, add comment

bafdb79

Refactor action button css

26f928a

Rename CSS classname portion to supplied-actions

167669b

Fix mailbox position calc

8913a53

Restrict to only appending new mailboxes to list

9429660

Anchor remove button to right in desktop

382de28

Sangeeth Sudheer and others added 2 commits August 26, 2021 18:04

Merge remote-tracking branch 'upstream/trunk' into update/add-remove-…

4754d2e

…mailbox-design-changes

Ensure we use consistent mailbox header color

bb84b0b

daledupreez approved these changes Aug 26, 2021

View reviewed changes

runofthemillgeek mentioned this pull request Aug 26, 2021

Domains: Add skip button in professional email mailbox list form in upsell page #55765

Merged

daledupreez merged commit 6194a72 into Automattic:trunk Aug 26, 2021

daledupreez deleted the update/add-remove-mailbox-design-changes branch August 26, 2021 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emails: Mailbox list and add/remove button design changes #55529

Emails: Mailbox list and add/remove button design changes #55529

runofthemillgeek commented Aug 16, 2021 •

edited

Loading

a8ci18n Aug 16, 2021

stephanethomas Aug 16, 2021

saygunnyc Aug 25, 2021

daledupreez left a comment

daledupreez Aug 20, 2021

runofthemillgeek Aug 20, 2021

daledupreez Aug 20, 2021

runofthemillgeek Aug 20, 2021

saygunnyc Aug 25, 2021

runofthemillgeek commented Aug 20, 2021

daledupreez left a comment

daledupreez Aug 23, 2021

runofthemillgeek Aug 23, 2021

daledupreez Aug 23, 2021

runofthemillgeek Aug 23, 2021

daledupreez Aug 23, 2021

runofthemillgeek Aug 23, 2021

daledupreez Aug 23, 2021

daledupreez Aug 23, 2021

daledupreez commented Aug 23, 2021

runofthemillgeek commented Aug 23, 2021

daledupreez commented Aug 25, 2021

saygunnyc commented Aug 25, 2021

daledupreez commented Aug 26, 2021

runofthemillgeek commented Aug 26, 2021

daledupreez left a comment

runofthemillgeek commented Aug 26, 2021

saygunnyc commented Aug 26, 2021

a8ci18n commented Aug 26, 2021

a8ci18n commented Sep 5, 2021

-							{ translate( 'Mailbox %(index)s', {
-								args: { index: String( index + 1 ).padStart( 2, '0' ) },
-							} ) }
+							{ translate( 'Mailbox %(position)s', {
+								args: { position: String( index + 1 ).padStart( 2, '0' ) },
+								comment: '%(position)s is the position of the mailbox in a list, e.g. Mailbox 1, Mailbox 2, etc',
+							} ) }

Emails: Mailbox list and add/remove button design changes #55529

Emails: Mailbox list and add/remove button design changes #55529

Conversation

runofthemillgeek commented Aug 16, 2021 • edited Loading

Changes proposed in this Pull Request

Testing instructions

Screenshots

Mobile (/email/:domain/purchase/:siteSlug)

Desktop (/email/:domain/purchase/:siteSlug)

Mobile (/email/:domain/titan/new/:siteSlug)

Desktop (/email/:domain/titan/new/:siteSlug)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daledupreez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runofthemillgeek commented Aug 20, 2021

daledupreez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daledupreez commented Aug 23, 2021

runofthemillgeek commented Aug 23, 2021

daledupreez commented Aug 25, 2021

saygunnyc commented Aug 25, 2021

daledupreez commented Aug 26, 2021

runofthemillgeek commented Aug 26, 2021

daledupreez left a comment

Choose a reason for hiding this comment

runofthemillgeek commented Aug 26, 2021

saygunnyc commented Aug 26, 2021

a8ci18n commented Aug 26, 2021

a8ci18n commented Sep 5, 2021

runofthemillgeek commented Aug 16, 2021 •

edited

Loading