Skip to content
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

Wrap inline remove_from_cart script in IIFE #310

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Oct 4, 2023

Changes proposed in this Pull Request:

Wrap inline remove_from_cart script in IIFE
To avoid syntax errors for const re-declaration.
Quick fix for https://wordpress.org/support/topic/javascript-output-twice/#post-17098290
7098382-zd-a8c

This is a regression from #283

Checks:

  • Does your code follow the WordPress coding standards?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Screenshots:

Detailed test instructions:

Global
  1. Enable cart tracking in /wp-admin/admin.php?page=wc-settings&tab=integration&section=google_analytics
  2. Open the shop page not as admin
  3. Add something to the cart
  4. Open cart page
  5. Open the browser console
  6. Check that there is no selector global variable.
Duplicate exectuion
  1. Enable cart tracking in /wp-admin/admin.php?page=wc-settings&tab=integration&section=google_analytics
  2. Add mini-cart widget to the cart page
  3. Open the shop page not as admin
  4. Add something to the cart
  5. Open cart page
  6. Open the browser console
  7. Check that there is no syntax errors

Additional details:

  1. I think we can actually move that script to the external shared file, but I'd leave it for future improvement and ship a quick fix sooner than later.

Changelog entry

Fix - JS syntax error on pages with cart and mini-cart rendered, which was causing purchases and cart removals not to be tracked.

@tomalec tomalec self-assigned this Oct 4, 2023
@github-actions github-actions bot added changelog: fix Took care of something that wasn't working. type: bug The issue/PR is a confirmed bug. labels Oct 4, 2023
@tomalec tomalec requested a review from a team October 4, 2023 20:58
Copy link
Contributor

@martynmjones martynmjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @tomalec, thanks for the quick fix!

Tested locally and confirmed that this change resolves the syntax warning so LGTM ✅

@tomalec tomalec merged commit 470dc91 into trunk Oct 5, 2023
7 checks passed
@tomalec tomalec deleted the fix/global-const branch October 5, 2023 09:51
@puntope puntope mentioned this pull request Oct 10, 2023
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue/PR is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants