WooPayments payment method logos component

timmy5685 commented 3 months ago

Description

Create shared components for the WooPayments payment method logos so they can be reused and are consistent across all places.

@elizaan36 has created 3 versions of the component: VLWDmeuIXQ7XAK4flyqTFO-fi-2196_11373

The "Expanded" component should be implemented in the following locations:

Connect Page V1 - Located in Core (payments nav item)
Connect Page V2 - Located in Core (Get paid task page)
Connect Page V3 - Located in client
Payments Settings Page Marketing Tile (requires WooPayments to be installed, but disabled)

The "Abbreviated 1" component should be implemented in the following location:

Payments Settings Page (WooPayments not installed)

Acceptance criteria

3 variations of the shared component are created
Pages specified above use their respective components

Designs

WooPayments payment method logos shared component variations: VLWDmeuIXQ7XAK4flyqTFO-fi-2196_11373

timmy5685 commented 3 months ago

@elizaan36 - do you think we should wait to create the "Abbreviated 2" version until we have a place that it's needed?

@dmallory42 - will creating / not creating this version have a serious impact on the effort?

dmallory42 commented 3 months ago

@dmallory42 - will creating / not creating this version have a serious impact on the effort?

Good question! I would say it may have a small impact on the effort, but I also think it probably saves time in the long run to think about all the possible variations of the component that we may need in future at this point. 🙂

also cc. @anu-rock as this issue probably needs to be on our radar soon!

elizaan36 commented 3 months ago

Thanks for creating the issue, @timmy5685

do you think we should wait to create the "Abbreviated 2" version until we have a place that it's needed?

To clarify, it's one component with three variants. The thinking is that the appropriate variant is presented depending on the device size. So Expanded and Abbreviated 2 could appear in the same placement on Payments Connect depending on the viewport. Here's a mockup of Abbreviated 2 for Payments Connect on mobile.

Payments Connect - WP Installed, WPCOM connection required mobile

I think abbreviated 2 would also need to be used for the Payments settings example, when WooPayments isn't installed.

I'm in the process of putting up a P2 with all of the details.

anu-rock commented 3 months ago

Update: The P2 that @elizaan36 mentioned above was published at paJDYF-dA0-p2.

dpaun1985 commented 2 months ago

Bumped estimates to 5 SPs due to:

setup of woocommerce env.
the component should be created in both woo payments and woocomerce repos
find out how to access the screens where the component needs to be implemented

dpaun1985 commented 2 months ago

@timmy5685 , for "Payments Settings Page (WooPayments not installed)" the logos are pulled from woocommerce.com . I don't think it's a good ideea to overwrite it. Should we leave the logos as they are for this screen?

timmy5685 commented 2 months ago

@dpaun1985 - we're going to need this component in core with the upcoming changes to the onboarding flow. Can we create this component in the client and in core? or is there a way to share the component across the client and core?

dpaun1985 commented 2 months ago

@dpaun1985 - we're going to need this component in core with the upcoming changes to the onboarding flow. Can we create this component in the client and in core? or is there a way to share the component across the client and core?

I started to implement the component in woocommerce core in order to be reused in the onboarding flow.

There was a problem with reusing it in woo payments, as we need to upgrade woopayments to use node V 20, so I duplicated the component also in woo payments for now. [ ref: p1718353521468869-slack-C03KTTK2YMA ]

For the "Payments Settings Page (WooPayments not installed)" screen , this is a particular case. Information such as the WooPayments payment method title, description, and subtitle is pulled from here. We could overwrite the logos using the new component, but I don't think we should.

cc @anu-rock @vladolaru - maybe you can share your thoughts.

anu-rock commented 2 months ago

@dpaun1985 Did I understand your question correctly? From your comment, it seems we are using the above woocommerce.com JSON to conditionally display PM icons. Are you asking whether we should ignore the sub_title value for "woocommerce_payments" in the JSON in favor of our new component?

<img class="wcpay-visa-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/visa.svg" alt="Visa"><img class="wcpay-mastercard-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/mastercard.svg" alt="Mastercard"><img class="wcpay-amex-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/amex.svg" alt="Amex"><img class="wcpay-googlepay-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/googlepay.svg" alt="Googlepay"><img class="wcpay-applepay-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/applepay.svg" alt="Applepay">

I may be mistaken, but isn't that the objective of this enhancement (removing PM icon inconsistency across surfaces)?

dpaun1985 commented 2 months ago

@dpaun1985 Did I understand your question correctly? From your comment, it seems we are using the above woocommerce.com JSON to conditionally display PM icons. Are you asking whether we should ignore the sub_title value for "woocommerce_payments" in the JSON in favor of our new component?

<img class="wcpay-visa-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/visa.svg" alt="Visa"><img class="wcpay-mastercard-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/mastercard.svg" alt="Mastercard"><img class="wcpay-amex-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/amex.svg" alt="Amex"><img class="wcpay-googlepay-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/googlepay.svg" alt="Googlepay"><img class="wcpay-applepay-icon wcpay-icon" src="https://woocommerce.com/wp-content/plugins/wccom-plugins/payment-gateway-suggestions/images/icons/applepay.svg" alt="Applepay">

I may be mistaken, but isn't that the objective of this enhancement?

Yes, this is the question. I am asking because I see that JSON as the source of truth on displaying the payment method. Ignoring the sub_title could create confusion ( or inconsistency if it is used in other places ). That's why I'm not sure if we should change it.

So, your suggestion is to ignore the sub_title and use the new component ?

anu-rock commented 2 months ago

@dpaun1985 Thanks for confirming my understanding. I will fall back to @timmy5685 on this one.

anu-rock commented 2 months ago

In the meantime, where else are we using the JSON to display PM icons?

dpaun1985 commented 2 months ago

In the meantime, where else are we using the JSON to display PM icons?

In woocommerce I could not identify other places.

timmy5685 commented 2 months ago

There was a problem with reusing it in woo payments, as we need to upgrade woopayments to use node V 20, so I duplicated the component also in woo payments for now. [ ref: p1718353521468869-slack-C03KTTK2YMA ]

Is this temporary? or will we be able to consolidate these to a shared component in the next release?

Yes, this is the question. I am asking because I see that JSON as the source of truth on displaying the payment method. Ignoring the sub_title could create confusion ( or inconsistency if it is used in other places ). That's why I'm not sure if we should change it.

So, your suggestion is to ignore the sub_title and use the new component ?

@anu-rock is correct in that goal of this new component is to have consistency across core and WooPayments. If there are changes in the future, we'll make them once in the component and it will update everywhere inside core and WooPayments. I don't think we should be linking ourselves or be dependent on Woocomerce.com. I'm not the expert here - but I think we should be deprecating the old way of doing this.

dpaun1985 commented 2 months ago

Is this temporary? or will we be able to consolidate these to a shared component in the next release?

It's a temporary solution. As soon as we will update the node version, we can use the core component in both places.

dpaun1985 commented 2 months ago

PR for WooPayments: https://github.com/Automattic/woocommerce-payments/pull/8977
PR for WooCommerce core: https://github.com/woocommerce/woocommerce/pull/48598

timmy5685 commented 2 months ago

@dpaun1985 - sounds good. Do we have a separate issue for switching to the single shared component so I can follow that one too? or is it one the the two you shared above?

dpaun1985 commented 2 months ago

@dpaun1985 - sounds good. Do we have a separate issue for switching to the single shared component so I can follow that one too? or is it one the the two you shared above?

I created #9002 .

elazzabi commented 2 months ago

Update regarding the SVG-in-JS issue (context: pe4Hk0-1d6-p2#comment-1459)

I checked with @frosso to ask a few clarifying questions, and to get some feedback about the time commitment here based on WooPay's experience. The following is a summary:

How do SVGs get referenced?

The list of SVGs get built into a map file (via a webpack plugin dev-packages/svg-manager/webpack-plugin.js), injected in the header of the page (woopay-header.php#L24), and then referenced in the JS (using the href prop in SvgManager component)

How much time will this take to implement in WooPayments?

It's been a while since I implemented it, but I think most of the changes can be copy/pasted to port it over. However, I'm not sure how the SVGs should be added to the DOM when using WooPayments.

I don't think we should dump all the SVGs to the DOM. Some SVGs might be relevant only to the admin area, others only to the frontend.

For now, our SVGs are needed in the admin area. So we can build the map, inject it to the DOM via WordPress hooks in the admin area, and use it there. If for some reason we need to build SVGs for the customer-facing part, we'll need to see how to build two different maps.

mordeth commented 1 month ago

For now, our SVGs are needed in the admin area. So we can build the map, inject it to the DOM via WordPress hooks in the admin area, and use it there. If for some reason we need to build SVGs for the customer-facing part, we'll need to see how to build two different maps.

We already have a webpack configuration that loads SVGs as assets, preventing the bundle from becoming bloated. To use it we simply need to add ?asset to the end of each SVG’s import path.

I’ve guided @dpaun1985 to use this method, and it successfully removed the extra size.

No additional component is needed to render SVGs as sprites, so I have closed the #9029 spike.

Automattic / woocommerce-payments