Skip to content
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

Fix mobile display of the new email plans page #52912

Merged
merged 8 commits into from May 18, 2021

Conversation

daledupreez
Copy link
Contributor

@daledupreez daledupreez commented May 17, 2021

Changes proposed in this Pull Request

This PR includes the following improvements:

  • We now hide the "Admin" badge when the user is on narrower screens (this was in the original designs)
  • We now use the same font size for the "Expires" text and the auto-renew label
  • When the screen gets narrower, we push the "Renew now" button to the right, and allow the auto-renew label to shift onto the next line if needed
  • The mailbox-level ellipsis menu is now aligned with the top text, and spacing for it is responsive based on the top row of the mailbox card
  • The left spacing for mailboxes with warnings has been fixed for wider screens -- it was too narrow and left mailboxes with warnings too far to the left

Testing instructions

  • Open the UI locally or via the live branch
  • Navigate to /email/:siteSlug
  • Click on an email account that has one or more admin users (which should be most Google Workspace, G Suite, or Professional Email accounts) -- also ensure that the email plan for the subscription shows the auto-renew toggle (which is controlled by whether there is a payment method linked to the subscription)
  • For various browser widths, but especially mobile widths (< 600px wide) and tablet widths (< ~1000px), verify the following:
    • We don't show the "Admin" badge across the card (see the screenshots below for a visual)
    • We show the "Auto-renew" text in the same size font as the "Expires" text (this should apply across all screen sizes, including desktop)
    • For narrower widths (especially under ~400px), the subscription management options below the plan header respond well, with the Auto-renew toggle shifting to the next line at narrower widths.
    • The ellipsis menu should be aligned with the top row of text in all cases
    • The mailbox name should not overlap with the ellipsis menu, especially as the screen gets really narrow
  • Navigate to the plan page for a domain with email forwarding and an unverified email address for a mailbox. (You can create a new forward if needed.)
  • Verify that on narrow screens, the warning content for the mailbox and the ellipsis menu don't interfere with each other. (You may need to test that using Update action menu items in email plans page #52930, which is derived from this branch.)

Screenshots

Before

Screenshot 2021-05-17 at 10 54 25

After

Screenshot 2021-05-17 at 10 55 47

Left spacing for warnings
Before After
Screenshot 2021-05-18 at 10 01 37 Screenshot 2021-05-18 at 10 01 56
Ellipsis menu alignment and overflow
Before After
Screenshot 2021-05-18 at 10 09 30 Screenshot 2021-05-18 at 10 15 41

@daledupreez daledupreez requested a review from a team May 17, 2021 10:00
@daledupreez daledupreez self-assigned this May 17, 2021
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 17, 2021
@github-actions
Copy link

@matticbot
Copy link
Contributor

matticbot commented May 17, 2021

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~11 bytes added 📈 [gzipped])

name   parsed_size           gzip_size
email        +85 B  (+0.0%)      +11 B  (+0.0%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@daledupreez daledupreez force-pushed the fix/mobile-display-email-plans branch from 4b85a16 to 56ba23c Compare May 17, 2021 11:26
@daledupreez daledupreez added the Email management Issues with email management screens label May 18, 2021
Copy link
Contributor

@stephanethomas stephanethomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works fine except for admin badges which are displayed too far from mailboxes:

screenshot

This is caused by some right padding we always apply to mailboxes as a way - I suppose - to keep them from overlapping the ellipsis menu on narrow viewports. I think we could address that by relying on flex for positioning that particular menu instead of using absolute coordinates (which frankly we should avoid as much as possible as this is always a source of problems when making a design responsive).

client/my-sites/email/email-management/style.scss Outdated Show resolved Hide resolved
@stephanethomas
Copy link
Contributor

Hum, I see now that this may be blocking some other enhancements so I'm totally fine with merging that pull request as it-is, and eventually addressing the issue I've raised in a future iteration.

@daledupreez
Copy link
Contributor Author

This is caused by some right padding we always apply to mailboxes as a way - I suppose - to keep them from overlapping the ellipsis menu on narrow viewports. I think we could address that by relying on flex for positioning that particular menu instead of using absolute coordinates (which frankly we should avoid as much as possible as this is always a source of problems when making a design responsive).

For context, the rest of the card is already using flex, which is part of the problem - the other items need to flow differently from this one. That was actually the source of the problem -- when the other items were using flex-direction: column; we want the padding/space on the top item aligned with the menu, but not the other items. But when we shift to flex-direction: row; for the menu's siblings, the parent element needs the padding, as the first child element is now left-aligned.

@stephanethomas
Copy link
Contributor

Approved 🚢.

@daledupreez daledupreez merged commit 8d80d60 into trunk May 18, 2021
@daledupreez daledupreez deleted the fix/mobile-display-email-plans branch May 18, 2021 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Email management Issues with email management screens [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants