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

WooPayments payment method logos component #8808

Closed
timmy5685 opened this issue May 9, 2024 · 20 comments · Fixed by #8977
Closed

WooPayments payment method logos component #8808

timmy5685 opened this issue May 9, 2024 · 20 comments · Fixed by #8977
Assignees
Labels
component: onboarding Onboarding merchant in KYC, dev vs live, etc focus: account lifecycle priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. type: enhancement The issue is a request for an enhancement.

Comments

@timmy5685
Copy link

timmy5685 commented May 9, 2024

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)
    image

  • Connect Page V2 - Located in Core (Get paid task page)
    image

  • Connect Page V3 - Located in client
    image

  • Payments Settings Page Marketing Tile (requires WooPayments to be installed, but disabled)
    image

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

  • Payments Settings Page (WooPayments not installed)
    image

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
Copy link
Author

@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
Copy link
Contributor

@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
Copy link

elizaan36 commented May 10, 2024

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 anu-rock added type: enhancement The issue is a request for an enhancement. priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. component: onboarding Onboarding merchant in KYC, dev vs live, etc labels May 23, 2024
@anu-rock
Copy link
Contributor

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

@dpaun1985
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

@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
Copy link
Contributor

anu-rock commented Jun 19, 2024

@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
Copy link
Contributor

dpaun1985 commented Jun 19, 2024

@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
Copy link
Contributor

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

@anu-rock
Copy link
Contributor

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

@dpaun1985
Copy link
Contributor

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

In woocommerce I could not identify other places.

@timmy5685
Copy link
Author

timmy5685 commented Jun 20, 2024

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
Copy link
Contributor

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
Copy link
Contributor

@timmy5685
Copy link
Author

timmy5685 commented Jun 21, 2024

@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
Copy link
Contributor

@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
Copy link
Member

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:

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

  1. 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
Copy link
Contributor

mordeth commented Jul 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: onboarding Onboarding merchant in KYC, dev vs live, etc focus: account lifecycle priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. type: enhancement The issue is a request for an enhancement.
Projects
None yet
7 participants