-
Notifications
You must be signed in to change notification settings - Fork 69
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
Updated blocks checkout payment method label design #9483
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: +324 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me and works well across different browser resolutions. 🙌 I have one small comment about the location of the icon asset URL on which I'd love to hear your thoughts. Other than that LGTM.
let icon = | ||
upeAppearanceTheme === 'night' ? upeConfig.darkIcon : upeConfig.icon; | ||
if ( upeName === 'card' ) { | ||
icon = GenericCardIcon; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can foresee us having a lot of additional conditionals here to support other updated payment methods. Can we not simply update this icon_url
property on the CC_Payment_Method
class? I think this icon property is only used at checkout and reusing this parameter would allow us to set this asset in a single place and reuse it in the shortcode checkout as well. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also used on the Settings page, so we would need a conditional there if we updated the icon_url. I think using it here is better as we're only using it while we can't remove the brand icons from Stripe. We will still need this conditional when we update it to the multiple brand icons version, as we can't have multiple icons in icon_url
.
I don't think this will be the case for any other payment method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also used on the Settings page
On the settings page, AFAIA, that icon is imported separately here from where it is used from this map. WC_Payment_Gateway::get_icon()
is used in two places:
WC_Payment_Gateway::get_icon_url()
- This is only used by
WC_Payments_Checkout
here to provide this to the client, where I believe thisicon
property is only consumed by our checkout client code that you've currently adapted.
- This is only used by
WC_Payment_Gateway::get_theme_icon()
- This is only used to set the gateway's
icon
property here, which is actually used to set the icon for the shortcode checkout gateway.
- This is only used to set the gateway's
As I see it, we can either change the CC_Payment_Method
icon_url
property here to use the generic icon and share this between shortcode and blocks checkouts or we can ignore this property and set the asset manually on the client, as you've done in this PR. If we choose the second approach, we may no longer have any use for the icon_url
property on the CC_Payment_Method
class however, though that may not be such a bad thing.
In any case, I don't see that much need to fret over this decision in this PR. I've just created #9487 as a follow-up, so we can let whoever picks up that issue find an answer to this dilemma there. We could still possibly use both approaches and keep the icon_url
solely for the shortcode checkout, though this seems like it might get a little redundant.
I don't think this will be the case for any other payment method.
On further consideration, this also seems like a valid point.
We will still need this conditional when we update it to the multiple brand icons version, as we can't have multiple icons in icon_url.
This too, so I think the approach here seems like a valid one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! I'm sorry. I got confused because my first thought was to replace the cc.svg file with the new svg, which affected the settings icon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gpressutto5 @FangedParakeet double checking here that we'll also be increasing the payment method row height per Nikki's design to give more breathing room to the BNPL messaging which is very squashed on shortcode 2 and 3 column checkouts. |
Fixes #9474
Fixes #9475
Changes proposed in this Pull Request
This PR adds the Test mode label and updates the Credit Card logo for the Payment Method labels on the Blocks checkout.
Testing instructions
npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge