-
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
Unify payment method icon design #7560
Conversation
We still need to fix some failing tests, mostly just recreating snapshots. Also, this PR needs to address the use cases where assets are used as CSS backgrounds. I might skip it since there are no problems with alignment or borders. |
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.
Early feedback: looks good! Leaving some comments to look into (minor ones).
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.
client/payment-methods-map.tsx
Outdated
@@ -200,11 +197,10 @@ const PaymentMethodInformationObject: Record< | |||
affirm: __( 'Affirm', 'woocommerce-payments' ), | |||
}, | |||
description: __( | |||
// translators: %s is the store currency. | |||
'Allow customers to pay over time with Affirm. Available to all customers paying in %s.', | |||
'Expand your business with Affirm', |
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 noticed we are changing the description for Affirm and Afterpay, is this intended? Asking because for Klarna we are not changing this description.
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.
Not intended, thanks for pointing out. Could be incorrect merge from develop
. I'll look into that.
@mgascam Thank you for pointing out the missing LinkIcon. I've implemented all requested changes. Just need to resolve failing tests (in most cases recreating snapshot) |
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: +4.83 kB (0%) Total Size: 1.44 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.
Everything looks good and consistent on the Settings and Transactions screens! Good work!
Some use cases like removing PM modal relies on additional classes added to the root img
Current grid component is not resiable and brakes the design for enabling Credit card modal
Fixes #7183
Changes proposed in this Pull Request
Unify the design for Payment method and Card icon design.
Testing instructions
TODO: Need to list all use places. Quite hard to gather them all
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