-
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
Fix invalid value error for paymentMethodMessaging in product details page #7153
Fix invalid value error for paymentMethodMessaging in product details page #7153
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: +109 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
213ec27
to
c30f9cf
Compare
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.
Like the direction, but we shouldn't be updating element if we can not reliable calculate price. See my comments.
This reverts commit 9bc87d5.
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.
Pre-approving, as handed further reviews to @KarlisJ.
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.
Overall, when looking strictly at the changes and comparing old vs new code it's a nice improvement, I like the changes.
But my first though seeing the resetBnplPaymentMessage
and quantityInput.on( 'change',
was "why can't we trigger the change
event from resetBnplPaymentMessage
?" . I believe that's the "jQuery approch" for less code duplication.
On the surface level it seems that both blocks operate with different values, but digging deeper it seems to me that it's not true.
In resetBnplPaymentMessage
we access productVariations.base_product.amount
but in the change handler we access productVariations[ productId ].amount
. Looking at the source productId
that we define at the top will always be base_product
so essentially both blocks perform the same action.
Or have I misunderstood something?
…hub.com:Automattic/woocommerce-payments into fix/6819-payment-method-messaging-invalid-value
…hub.com:Automattic/woocommerce-payments into fix/6819-payment-method-messaging-invalid-value
Thanks @KarlisJ I looked into your comment about variations and refactored the code. Also fixed some buggy scenarios where the messaging element was not being updated. Please take a look and let me know if you like it better.
Apologies but I'm not following you here. |
Before your last commits it seemed that both Now I see that they do not perform same action thus can't be reused is such way. |
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.
LGTM, approving with one minor comment.
Fixes #6819 (reopened)
This issue has been reopened. We received reports of errors in the plugin in the product details page after the previous fix was released. This time, errors seem to happen due to merchants customizing this page by removing the quantity input.
Changes proposed in this Pull Request
Testing instructions
To replicate the issue we need to remove the quantity input from the product details page. This can be achieved by several means. We got reports of merchants using a page builder. I was able to replicate the issue locally, using this hook
woocommerce_is_sold_individually
. In the functions.php file of the Storefront theme add the following:paymentMethodMessaging
.custom_woocommerce_remove_quantity_input
)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