-
Notifications
You must be signed in to change notification settings - Fork 68
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
Comments
@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? |
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! |
Thanks for creating the issue, @timmy5685
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. I'm in the process of putting up a P2 with all of the details. |
Update: The P2 that @elizaan36 mentioned above was published at paJDYF-dA0-p2. |
Bumped estimates to 5 SPs due to:
|
@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. |
@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. |
@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
I may be mistaken, but isn't that the objective of this enhancement (removing PM icon inconsistency across surfaces)? |
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 So, your suggestion is to ignore the |
@dpaun1985 Thanks for confirming my understanding. I will fall back to @timmy5685 on this one. |
In the meantime, where else are we using the JSON to display PM icons? |
In woocommerce I could not identify other places. |
Is this temporary? or will we be able to consolidate these to a shared component in the next release?
@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. |
It's a temporary solution. As soon as we will update the node version, we can use the core component in both places. |
|
@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 . |
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:
The list of SVGs get built into a map file (via a webpack plugin
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 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. |
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:
Acceptance criteria
Designs
WooPayments payment method logos shared component variations: VLWDmeuIXQ7XAK4flyqTFO-fi-2196_11373
The text was updated successfully, but these errors were encountered: