Migrate to custom containers and better elements scoping #9782
+345
−230
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: +103 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
timur27
commented
Nov 21, 2024
client/checkout/utils/upe.js
Outdated
Comment on lines
191
to
192
| ? 'wc-woocommerce_payments-payment-token-new' | ||
| : `wc-woocommerce_payments_${ paymentMethodType }-payment-token-new`; |
There was a problem hiding this comment.
This is a small refactor that I believe makes the code more readable by removing the prefix and suffix concepts
added 8 commits
November 22, 2024 12:17
timur27
changed the title
timur27
commented
Nov 22, 2024
timur27
commented
Nov 22, 2024
| @@ -83,9 +58,11 @@ export const getHiddenBillingFields = ( enabledBillingFields ) => { | |||
| }; | |||
| }; | |||
|
|
|||
| export const getUpeSettings = () => { | |||
| export const getUpeSettings = ( paymentMethodType ) => { | |||
There was a problem hiding this comment.
I decided to add paymentMethodType here because the current idea behind getUpeSettings is that we run it for every gateway/payment method and it should theoretically check for every payment method type. So we let shouldIncludeTerms to scope by the wcpay-upe-form which has the payment method type equal to the one coming as a parameter here, and check from there
timur27
commented
Nov 22, 2024
client/checkout/utils/upe.js
Outdated
| const upeContainer = document.querySelector( | ||
| '.payment_method_woocommerce_payments_' + paymentMethodType | ||
| ); | ||
| const upeContainer = upeElement.closest( 'li.wc_payment_method' ); |
There was a problem hiding this comment.
Traversing up trying to find the first parent which will be ready to be used so that we can properly hide/show the gateway
timur27
commented
Nov 22, 2024
| if ( ! empty( $this->gateway->get_description() ) ) : ?> | ||
| <p><?php echo wp_kses_post( $this->gateway->get_description() ); ?></p> | ||
| ?> | ||
| <div class="wcpay-upe-form" |
There was a problem hiding this comment.
This is the main wrapper, which should serve as the base for all DOM traversal moving forward. Ideally, we should aim to locate this container in the DOM and then traverse within it to find the relevant elements needed for our purposes
added 6 commits
November 25, 2024 09:45
timur27
force-pushed
the
scope/payment-elements-selectors
branch
from
14c21f6 to
6a2bc69
Compare
timur27
force-pushed
the
scope/payment-elements-selectors
branch
from
3defc99 to
7e7de87
Compare
added 2 commits
November 29, 2024 14:19
frosso
reviewed
Nov 29, 2024
frosso
reviewed
Nov 29, 2024
frosso
reviewed
Dec 2, 2024
frosso
approved these changes
Dec 2, 2024
|
After the reviews so far, I thought I would attach the most recent testing round results I performed with the Vendrify theme. Screen.Recording.2024-12-04.at.10.26.47.mov |
mdmoore
approved these changes
Dec 4, 2024
There was a problem hiding this comment.
Everything is testing well with the changes and checkouts are processing as expected. Checkout is working on Vendrify, but a couple things (which I'm not sure if are meant to be fixed in this PR) stand out.
Vendrify theme only
- Dynamic payment method filtering isn't working during checkout. Only the payment methods supported by the country set at page load are displayed and changing the billing country has no effect on the payment methods.
- Pay for Order - I think is beyond our control but surfacing anyways.
a. If WooPayments is active, it's always displayed as the top payment method, regardless of payment method ordering at WooCommerce > Payments
b. All other payment methods are hidden except for bullets. This includes WooPayments LPMs.
With the above in mind, since checkout is working with the new scoping and containers and checkout in Vendrify is now working, I think we can approve this and look at the above isssues separately if needed.
|
Thanks for the review and such a thorough testing, @mdmoore! 🚀 There are things with that theme that could work better, I agree. I think it would be OK to focus on fixing the critical flow, which is performing a purchase, and keep the rest part of the particular theme implementation (ideally) or as a separate issue (less preferred because I think there are limits to what we can do in terms of theme compatibility, as fixing one theme might break others). By the way, this theme is not being maintained anymore, from what I understand. The dynamic payment method filtering issue might be related to #9851, which I opened last week. It occurs even with some default testing themes. Maybe fixing this issue will fix the issue you encountered, but I'd focus first on the primary behavior and then on any potential user reports about problems with the theme. |
6 tasks
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 #7674
Changes proposed in this Pull Request
This PR creates a new div wrapper with a unique class name for WooPayments payment elements and uses it to search for elements within them. In addition, the direct targeting of WooCommerce elements has been replaced with targeting our element first and then traversing by looking at an element from that point.
This PR also fixes compatibility issue Vendrify theme had with WooPayments.
Testing instructions
Shortcode checkout
Add payment method & Pay for order
Themes
npm 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