Add the new payment method logos to the connect page #8977
Merged
Conversation
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
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: +897 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
elazzabi
previously requested changes
Jun 21, 2024
There was a problem hiding this comment.
Left a few requests for changes.
Also, the diff in size is a little high "+49.5 kB (+17%)", probably due to SVGs being components now...
This was shared in a comment in this p2: pe4Hk0-1d6-p2, I think it's worth double-checking to see if we can do better here.
…t-method-logo' into add/8808-woo-payment-method-logo
12 tasks
…t-method-logo' into add/8808-woo-payment-method-logo
mordeth
requested changes
Jul 2, 2024
There was a problem hiding this comment.
Thank you for working on this, Dan!
I have manually tested the component with different breakpoints and it works perfectly. Your approach of adjusting visible elements based on breakpoint width is a great idea.
Furthermore, I’ve provided some suggestions for naming and CSS enhancements.
added 2 commits
July 2, 2024 16:30
mordeth
reviewed
Jul 3, 2024
…t-method-logo' into add/8808-woo-payment-method-logo
mordeth
reviewed
Jul 9, 2024
mordeth
reviewed
Jul 9, 2024
…t-method-logo' into add/8808-woo-payment-method-logo
mordeth
approved these changes
Jul 9, 2024
|
hey @dpaun1985 There may be some extra CSS creating an additional outline on the icons. 👀 Here's what I'm seeing. Ideally we can match the implementation of the icon row in @mordeth 's PR to create a unified experience pre and post install. |
fixed. There were some last changes that made that outline appear. it should be like in the other PR right now |
|
Thanks @dpaun1985, LGTM! |
dpaun1985
dismissed
elazzabi’s stale review
already reviewed by Cvetan and Elizabeth
lovo-h
pushed a commit
that referenced
this pull request
Aug 1, 2024
Co-authored-by: Dan Paun <[email protected]> Co-authored-by: oaratovskyi <[email protected]> Co-authored-by: Cvetan Cvetanov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #8808
Changes proposed in this Pull Request
Create a new component that can be reused, to display the new payment method logos.
Design: https://www.figma.com/design/VLWDmeuIXQ7XAK4flyqTFO/Payments-Adoption-%26-Activation?node-id=2196-11373&t=7gXadrjGDmPqgpUP-0
Testing instructions
Force the WCPay plugin to act as disconnected from the WCPay Serveroption and save the settingsnpm run changelogto add a changelog file, choosepatchto 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