-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add checks and explicit errors when cart buttons are missing data #443
Conversation
to help debugging issues with other extensions manipulating WC Core add & remove from cart buttons or events.
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.
Thanks for the improvement. LGTM.
Commented on two minor concerns, and I believe it won't need another round even if there are related changes.
@@ -60,14 +60,20 @@ export function classicTracking( | |||
} | |||
// Get product ID from data attribute (archive pages) or value (single product pages). | |||
const productID = parseInt( | |||
button[ 0 ].dataset.product_id || button[ 0 ].value | |||
button?.[ 0 ]?.dataset.product_id || button?.[ 0 ].value |
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.
); | ||
|
||
if ( isNaN( productID ) ) { |
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.
Although parseInt
won't return a type other than number or NaN
, it would be better to avoid using isNaN
when possible because it has some confusing special-case behavior that leads to semantics ambiguity.
Maybe it could be replaced with Number.isNaN
.
Address #443 (comment) Fix grammar in the error message.
Thanks @eason9487 for the detailed review :) |
Changes proposed in this Pull Request:
Add checks and explicit errors when cart buttons are missing data
to help debugging issues with other extensions manipulating WC Core add & remove from cart buttons or events.
This would help identifying issues like the one reported at https://wordpress.org/support/topic/js-error-on-add-to-cart-when-logged-out/
Checks:
Screenshots:
Detailed test instructions:
document.querySelectorAll('[data-product_id]').forEach((e)=>e.removeAttribute('data-product_id'))
. 8. Remove item from mini cart using UIAdditional details:
Changelog entry