-
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
Merged
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4a0705c
WIP Updated blocks checkout label design
gpressutto5 7e23cc6
WIP Updated blocks checkout label design
gpressutto5 f8be59d
Using new icons for card only
gpressutto5 d464b91
Updated Card brand icons
gpressutto5 ddb4e3f
Using generic card icon for credit card
gpressutto5 06e410a
Added changelog entry
gpressutto5 622ef28
Removed unused icons
gpressutto5 2673bfa
Fixed css
gpressutto5 91ebb85
Merge branch 'develop' into blocks-checkout-label-design
gpressutto5 0241d4b
Updated icon_url instead of using conditional in the frontend
gpressutto5 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Significance: minor | ||
Type: add | ||
|
||
Redesigned the Payment Method labels for the Blocks Checkout |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 theCC_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.
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()
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.WC_Payment_Gateway::get_theme_icon()
icon
property here, which is actually used to set the icon for the shortcode checkout gateway.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 theicon_url
property on theCC_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.On further consideration, this also seems like a valid point.
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.