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.

Sign up for GitHub

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

Jump to bottom

Emails: Show discount star in the Google Workspace section in the Email Comparison page #59189

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Emails: Show discount star in the Google Workspace section in the Email Comparison page #59189

merged 1 commit into from
Dec 14, 2021

Conversation

stephanethomas
Copy link
Contributor

@stephanethomas stephanethomas commented Dec 14, 2021 •
edited
Loading

This pull request updates the Email Comparison page to show a star with a discount in the Google Workspace section when Google Workspace is on sale:

Before After
image image

Testing instructions

  1. Run git checkout add/google-workspace-star and start your server, or access a live branch
  2. Open the Emails page
  3. Click the Add Email button to access the Email Comparison page
  4. Assert that you see a star with a label in the Google Workspace section
  5. Assert that there is no display issue on mobile devices

@stephanethomas stephanethomas self-assigned this Dec 14, 2021
@stephanethomas stephanethomas requested a review from a team December 14, 2021 15:56
@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 Dec 14, 2021
@github-actions
Copy link

github-actions bot commented Dec 14, 2021 •
edited by matticbot
Loading

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-22361
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-22361&env=jetpack

@matticbot
Copy link
Contributor

matticbot commented Dec 14, 2021 •
edited
Loading

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

Sections (~66 bytes added 📈 [gzipped])

name     parsed_size           gzip_size
email         +366 B  (+0.1%)      +66 B  (+0.0%)
domains       +366 B  (+0.0%)      +66 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.

@stephanethomas stephanethomas force-pushed the add/google-workspace-star branch 2 times, most recently from 09e421a to 81c473a Compare December 14, 2021 16:17
AllTerrainDeveloper
AllTerrainDeveloper reviewed Dec 14, 2021
View reviewed changes
Copy link
Contributor

@AllTerrainDeveloper AllTerrainDeveloper left a comment

Choose a reason for hiding this comment

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

Looks good to me, I've noted some unimportant things :)

client/my-sites/email/email-providers-comparison/email-provider-card.jsx
@@ -42,16 +43,29 @@ function EmailProviderCard( {

const labelForExpandButton = expandButtonLabel ? expandButtonLabel : buttonLabel;

const showStar = detailsExpanded && starLabel;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I see sometimes we use "should" prefix in this kind of constants. What's the general rule for this?

Suggested change
const showStar = detailsExpanded && starLabel;
const shouldShowStar = detailsExpanded && starLabel;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a rule for that, at least I'm not aware of any one. I'm not a big fan of this notation for booleans though, as it is longer and doesn't really clarify anything in my opinion. I'll mention that discussion in our Slack channel to see what others think of that.

client/my-sites/email/email-providers-comparison/email-provider-card.jsx
'is-expanded': detailsExpanded,
'is-forwarding': providerKey === 'forwarding',
} ) }
image={ logo }
title={ title }
badge={ badge }
>
{ showStar && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Same nit as above

Suggested change
{ showStar && (
{ shouldShowStar && (

client/my-sites/email/email-providers-comparison/email-provider-card.jsx
return (
<PromoCard
className={ classnames( 'email-providers-comparison__provider-card', {
'has-star': showStar,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same nit as above

Suggested change
'has-star': showStar,
'has-star': shouldShowStar,

@AllTerrainDeveloper
Copy link
Contributor

EOT

It looks amazing even on mobile display at any size! Good job!
image

@stephanethomas stephanethomas merged commit b0e9d7e into trunk Dec 14, 2021
@stephanethomas stephanethomas deleted the add/google-workspace-star branch December 14, 2021 18:03
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 14, 2021
@a8ci18n
Copy link

a8ci18n commented Dec 14, 2021

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

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

@stephanethomas stephanethomas changed the title Emails: Show discount star in the Google Workspace section in the Ema… Emails: Show discount star in the Google Workspace section in the Email Comparison page Dec 20, 2021
@saygunnyc
Copy link

Sharing some notes regarding these,

  • First of all, please, please, please ALWAYS reach out to designers whenever you need help with design, or we can never achieve a consistent language and experience amongst our product, which may seem hard to measure at first sight but really has a negative effect on our product's success. We love, and respect your work and are ALWAYS happy to make ourselves available.
  • Repeating the same information over the same card – 2x "%50 Off" doesn't help with making the information easier to extract.
  • We don't use the green color when we display pricing.
  • We already have a badge designed for displaying recommended solutions, discounts, etc. Leaning towards re-utilizing existing components will take us one step forward in achieving a consistent experience. We REALLY shouldn't design randomly new components that solve the same problem.

Sharing what this page should look like in the interim:
We already have a designed badge, cards within the onboarding on Inbox, so whatever we do in the interim should also act as a bridge to that experience.

Cards

Link to Figma: https://www.figma.com/file/oIzh3T78bXjCC8GIeNLhH5/1.Onboarding---Email-Comparison-Page-i1?node-id=801%3A11309

cc'ing @poligilad-auto as she will be taking over emails experience along with domains starting from 2022.

@a8ci18n
Copy link

a8ci18n commented Dec 24, 2021

Translation for this Pull Request has now been finished.

@stephanethomas stephanethomas mentioned this pull request Mar 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AllTerrainDeveloper AllTerrainDeveloper AllTerrainDeveloper left review comments

Assignees

@stephanethomas stephanethomas

Labels
G Suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stephanethomas @matticbot @AllTerrainDeveloper @a8ci18n @saygunnyc