Use ProductIcon from @automattic/components instead of internal components

[delawski]

This PR is a follow-up work to recently deployed #38253 (see: pbtFFM-11-p2).

It replaces all usages of Calypso-specific ProductIcon and PlanIcon components with a common @automattic/components ProductIcon component.

Changes proposed in this Pull Request

• Introduce a utility getProductIconSlug() function for getting a mapping between plan or product slug and icon slug (af2b05f)
• Use common ProductIcon in MyPlanCard (af2b05f)
• Replace PlanIcon with ProductIcon in PlanFeaturesHeader (4f328d0)
• Replace PlanIcon with ProductIcon in Banner (69c3711)
• Replace PlanIcon with ProductIcon in PurchaseItem (6901f67)
• Replace PlanIcon with ProductIcon in ManagePurchase (51b0abf)
• Revert some of the changes done in Purchases: update the purchases for backup product.  #37448 to PurchaseItem and ManagePurchase (6901f67 and 51b0abf) since now Jetpack Backup icons are supported by the ProductIcon.
• Replace PlanIcon with ProductIcon in PlanThankYouCard (441fd7c)
• Replace PlanIcon with ProductIcon in UpgradeNudgeExpanded (831a537)
• Remove unused PlanIcon and ProductIcon components (1aa994d and f056782)

Testing instructions

Go to each of the following pages and confirm that the plan icons are displayed as expected (no regression):

• http://calypso.localhost:3000/devdocs/design/banner

• http://calypso.localhost:3000/devdocs/blocks/plan-thank-you-card

• http://calypso.localhost:3000/devdocs/blocks/upgrade-nudge-expanded

• http://calypso.localhost:3000/me/purchases

• http://calypso.localhost:3000/me/purchases/[site]/[purchase] (go to individual purchases of various plans and products)

• http://calypso.localhost:3000/plans/[site] (small icons in the plans grid on desktop)

• http://calypso.localhost:3000/plans/my-plan/[site] (icons in the My plan/My Products lists)

Fixes n/a

[matticbot]

Test live: https://calypso.live/?branch=replace/product-icon-with-a8c-component

[simison]

Since you anyway use getProductIconSlug() everywhere:

<ProductIcon slug={ getProductIconSlug( plan ) } />

...would it make sense to have component do it for you and do <ProductIcon plan={ plan } />

Something named getProductIconSlug feels like it would fit ProductIcon just fine? :-)

Not to block this PR — just maybe something to either follow up or change in the component before this PR, if you think it would make sense.

[matticbot]

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

App Entrypoints (~106 bytes removed 📉 [gzipped])

name                   parsed_size           gzip_size
entry-login                 -140 B  (-0.0%)      -26 B  (-0.0%)
entry-domains-landing       -140 B  (-0.0%)      -13 B  (-0.0%)
entry-main                   -60 B  (-0.0%)      -31 B  (-0.0%)
entry-jetpack-cloud          -60 B  (-0.0%)      -36 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~12780 bytes added 📈 [gzipped])

name                  parsed_size           gzip_size
settings-security         +1693 B  (+0.6%)    +1402 B  (+1.9%)
plugins                   +1693 B  (+0.4%)    +1352 B  (+1.3%)
domains                   +1692 B  (+0.2%)    +1279 B  (+0.6%)
themes                    +1609 B  (+0.4%)     +767 B  (+0.8%)
earn                      +1604 B  (+0.6%)     +542 B  (+0.7%)
stats                     +1592 B  (+0.2%)     +590 B  (+0.3%)
settings                  +1592 B  (+0.3%)     +544 B  (+0.4%)
reader                    +1592 B  (+0.4%)     +590 B  (+0.5%)
posts-custom              +1592 B  (+0.5%)     +590 B  (+0.7%)
posts                     +1592 B  (+0.5%)     +590 B  (+0.7%)
jetpack-connect           +1592 B  (+0.3%)     +626 B  (+0.5%)
hosting                   +1592 B  (+0.8%)     +668 B  (+1.2%)
home                      +1592 B  (+0.6%)     +529 B  (+0.8%)
activity                  +1592 B  (+0.3%)     +579 B  (+0.5%)
settings-performance      +1591 B  (+0.8%)     +550 B  (+1.0%)
marketing                 +1591 B  (+0.4%)     +624 B  (+0.6%)
checkout                  +1554 B  (+0.1%)     +692 B  (+0.2%)
purchases                 +1544 B  (+0.2%)     +290 B  (+0.1%)
theme                     +1508 B  (+0.5%)      -25 B  (-0.0%)
plans                      +620 B  (+0.1%)     +382 B  (+0.3%)
migrate                    +327 B  (+0.2%)       +1 B  (+0.0%)
customize                   -83 B  (-0.0%)     -392 B  (-0.8%)
devdocs                     +51 B  (+0.0%)      +10 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.

Async-loaded Components (~5934 bytes added 📈 [gzipped])

name                                              parsed_size           gzip_size
async-load-design-playground                          +1691 B  (+0.1%)    +1287 B  (+0.3%)
async-load-design-blocks                              +1652 B  (+0.1%)     +573 B  (+0.1%)
async-load-design                                     +1633 B  (+0.1%)     +429 B  (+0.1%)
async-load-signup-steps-plans-atomic-store            +1592 B  (+1.6%)     +625 B  (+2.4%)
async-load-signup-steps-plans                         +1592 B  (+1.0%)     +624 B  (+1.5%)
async-load-signup-steps-domains                       +1592 B  (+0.7%)     +574 B  (+1.1%)
async-load-my-sites-current-site-notice               +1592 B  (+3.5%)     +553 B  (+5.1%)
async-load-components-web-preview-component           +1592 B  (+0.4%)     +590 B  (+0.6%)
async-load-blocks-jitm-templates-sidebar-banner       +1592 B  (+7.9%)     +521 B  (+8.4%)
async-load-blocks-jitm-templates-notice               +1592 B  (+8.1%)     +517 B  (+8.5%)
async-load-blocks-jitm-templates-default              +1592 B  (+8.6%)     +522 B  (+8.9%)
async-load-blocks-product-purchase-features-list       -118 B  (-0.3%)     -881 B  (-8.2%)

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.
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.

[tyxla]

This is looking really great, nice work @delawski!

I tend to agree with @simison though - from a component usability perspective, I think it would make more sense to support all those additional product slugs. It would be much easier, than having to port that getProductIconSlug between codebases when reusing the component here and there.

What do you think?

[jsnajdr]

@simison @tyxla getProductIconSlug is a part of and uses data from lib/products-values. If the ProductIcon component in @automattic/components did the getProductionIconSlug itself, it depends on that data and the package needs to ship them somehow.

So, the question is: is it a good idea to ship a module like lib/products-values as a part or as a dependency of a React component library?

When something is used only inside the Calypso app, anything can depend on anything else without visible consequences. But when extracting standalone packages, we need to think about dependencies more carefully.

[tyxla]

Well, the only thing getProductIconSlug does is handle the case for some more slugs. If we just had those slugs defined in the component, without any dependency from lib/products-values, I don't see why we would need to ship lib/products-values as a module. I think about it as what we already have, but for twice more slugs, and don't see a good reason not to go that way. Perhaps I'm misunderstanding something?

[jsnajdr]

I think about it as what we already have, but for twice more slugs, and don't see a good reason not to go that way.

The main job that getProductIconSlug is doing now is to map the _MONTHLY, _YEARLY and _2_YEARLY variations to one product slug. The product icons don't care about the subscription length.

So you're proposing to use the constants from lib/plans/constants directly as ProductIcon slugs?

The structure of the product constants doesn't seem to follow any unified convention. There are values like:

• business-bundle-2y
• ecommerce-bundle
• jetpack_premium_monthly

Sometimes there are dashes (-), sometimes underscores (_), WP.com plans use the -bundle suffix...

[tyxla]

Yeah, that's what I'm proposing. I know we're not very consistent with the product slug naming, and I'm not super happy about it, but it's a done deal anyway, and we can't fix it easily 😉

It's not about convention, it's about the fact that these are product slugs, and it makes sense for them to match existing products for a product icon component. In other words, I'd expect to pass any product slug from the WP.com store, and get an icon for it from that component. If we need to have an adapter or facade between the slug and the component, then it's likely a bad component design because it's a bit more difficult and less intuitive to use.

I don't have super strong feelings about it, but I just think that having the component handle this is in favor of a more intuitive component design. And as long as we have good documentation for the product slugs, we should be good. After all, those product slugs are used in all of our products and platforms, so we can't run away from them 😉

[tyxla]

In short, here's a user story of why I think what I'm suggesting makes sense:

As a ProductIcon component user, I expect to pass a product slug, and get an icon in return.

rather than:

As a ProductIcon component user, I expect to pass an icon slug, and get an icon in return.

because if this slug is not a product slug, then why are we calling the component ProductIcon?

Also, I realized that what we're introducing is just another layer of product slugs, which aren't really product slugs. What are they really then? Just another layer of confusion if you ask me.

Just my $0.02 😉

[delawski]

I was torn a bit while implementing the getProductIconSlug function because of the reasons you raised here yesterday.

There are 2 valid points raised here, 2 options we can choose from:

1. Since ProductIcon is part of the @automattic/components, it should ideally be agnostic of any third-party libraries or naming conventions. It's up to the ProductIcon's consumers to accommodate to the interface it exposes, even though it lives within the same organization and in most cases only A8c codebases will use it. It's how we have it right now.
2. Since ProductIcon is meant to be used to display A8c product icons, it's expected that the consumer shouldn't care about the particular interface the component provides. The consumer wants to get an icon for an arbitrary product or plan slug. Given the fact that those slugs are common to the whole organization, it'd be relatively safe to hardcode them inside the ProductIcon component.

Pros for option 1:

• simple interface of the ProductIcon
• independence of other libraries and subscription length
• pretty slugs 😉

Pros for option 2:

• simple interface for the consumers (Calypso, Jetpack)
• partial independence of other libraries (slugs are hard-coded)
• no need for a facade in the consumer codebase

So far I was convinced that we should keep the ProductIcon interface as simple as possible and make it "pretty". Right now I can see that from the practical point of view the second option is more viable.

We don't have any real control over the product and plan slugs used throughout the organization, and there's no way to make them "pretty". Since we can't fight the chaos, let's embrace it.

While the A8c product slugs don't seem stick to any naming convention and are rather ugly, the good part about them is that they are the same across all codebases.

My inclination right now is to use the actual product slugs inside the ProductIcon so that consuming the component is as easy as it can be. Let's encapsulate the complexity inside the component instead of delegating this task to consumers.

What do you all think?

[tyxla]

Thanks @delawski I think that's a pretty good summary of where we stand there 👍

My inclination right now is to use the actual product slugs inside the ProductIcon so that consuming the component is as easy as it can be. Let's encapsulate the complexity inside the component instead of delegating this task to consumers.

As I already suggested above, I lean towards this too. I think that if we make a list of the cons, it will be even easier to make a decision 😉

[delawski]

I've updated the ProductIcon component so that product and plan slugs are directly supported now in #38536

[tyxla]

Marking this as "in progress" and "blocked" by #38536

[tyxla]

It would be so awesome if we could land this one @delawski @jsnajdr. What do we need to make it happen?

[delawski]

I'm going to rebase this and do a smoke test on IE11. I'd like to have someone else doing a smoke test on IE11 as well before merging since a potential issue may bring the production down on IE.

[delawski]

I've rebased and tested the calypso.live site on Win10/IE11 (BrowserStack. Everything seems to be working as expected. Having a second check done by someone else would be perfect.

[enejb]

In my testing this PR looks good on IE 11.

[delawski]

It seems the e2e tests fail for a reason. I'll try to have a look into this tomorrow.

[delawski]

@enejb @tyxla I've udpated the e2e tests affected (wp-e2e-tests-full-desktop and wp-e2e-tests-full-mobile) and they are passing now.

However, another test (wp-e2e-tests-full-jetpack) fails. After reviewing the log it looks like none of the errors is related to the updates I've done in this PR.

In this case, do you think it's safe to deploy?

[tyxla]

I gave this some testing and it looks/works great! I'm going to rebase, retest, and 🚢

Thanks a lot for it @delawski!

[tyxla]

Fixed the desktop and mobile e2e tests with b019798. The Jetpack e2e test failures are unrelated and are currently consistently failing on master and other branches, too.

This works well and LGTM 🚢