Coder Social home page Coder Social logo

Comments (15)

aaemnnosttv avatar aaemnnosttv commented on August 29, 2024 1

FWIW, I think we should avoid using trackEvent as the name to avoid confusion with our internal function by the same name.

from site-kit-wp.

tofumatt avatar tofumatt commented on August 29, 2024 1

@benbowler I think it makes sense to add those here. I'll do that in the PR, it's not a big deal to include 🙂

EDIT: Never mind, they were included later 🙂

from site-kit-wp.

tofumatt avatar tofumatt commented on August 29, 2024

From the context/ACs here, would there be a separate, inline script if the user was using two ICE scripts (eg. using two ICE-supported plugins)?

Why output the script inline instead of having a single, common script file we build and include whenever any ICE JS script is used?

from site-kit-wp.

10upsimon avatar 10upsimon commented on August 29, 2024

@tofumatt as per out comms offline, I think it makes sense to:

  • Have this function be contained within it's own script that we will enqueue
  • Register it as a dep of all singular plugin scripts
  • Load it async
  • Load all singular plugin tracking scripts async too

I'll update the AC accordingly.

from site-kit-wp.

tofumatt avatar tofumatt commented on August 29, 2024

Just one question really, when you say a “proxy function”, do you mean an actual JS Proxy? Or do you just mean that our scripts should include/reference the new trackEvent function and use it to send events?

If it’s the latter, the AC should specify that it should be available (on a Site Kit JS global I’d think).

If the former: why? 😅 Proxies when it’s just for our code seems odd. Maybe it’s just a wording thing that’s tripping me up here 😆

from site-kit-wp.

10upsimon avatar 10upsimon commented on August 29, 2024

Just one question really, when you say a “proxy function”, do you mean an actual JS Proxy?

@tofumatt no no, not an actual proxy :) I simply meant a helper function that abstracts our calls to gtag when we track events. Yes, this should be defined against the global _googlesitekit or similar var, but that seems more like IB territory. I'll update the AC to reflect the above.

from site-kit-wp.

tofumatt avatar tofumatt commented on August 29, 2024

Sounds good, moving to IB 👍🏻

from site-kit-wp.

10upsimon avatar 10upsimon commented on August 29, 2024

@zutigrm

  • Update includes/Modules/Ads.php
    • In setup_assets method:
      • If conversionInfra feature flag is enabled instantiate new Script registering the above JS file and pushing it to the $assets array.
      • It should include 'execution' => 'async' in the arguments.

I do not think this is correct, as Conversion Tracking is not specific to the Ads module, and can be turned on from GA4 without the ads module active, and vice-versa.

I think it may be better to register this script from within the Conversion_Tracking class. WDYT?

from site-kit-wp.

zutigrm avatar zutigrm commented on August 29, 2024

@10upsimon Yes that makes more sense. Thanks. IB updated

from site-kit-wp.

10upsimon avatar 10upsimon commented on August 29, 2024

Thanks @zutigrm IB ✅

from site-kit-wp.

benbowler avatar benbowler commented on August 29, 2024

Flagging here that @eugene-manuilov doesn't update the tracking for MailChimp or Popup Maker as these weren't merged at the time this ticket was started.

Should we hold back this ticket and address that here when these two are merged or update those separately?

from site-kit-wp.

mohitwp avatar mohitwp commented on August 29, 2024

QA Update ⚠️

  • Tested on dev environment.
  • Verified Optin Monster, WP Forms, MailChimp, Popup Maker and WooCommerce plugin events.
  • Verified that events tracked for WPForms OptIn Monster, Popup Maker, Mailchimp, and WooCommerce providers now tracked with _source: "site-kit".
  • Verified when conversionInfra feature flag is enabled.
  • Verified when Analytics is connected.
  • Verified when both Analytics and ads module are connected.
  • Verified when only Ads module is connected.

Note : As mentioned on #8553 (comment) for WooCommerce only 'add to cart' event is getting tracked.

@wpdarren I'm not able to test Contact Form 7 event on InstaWP and TasteWP due to same error. Can you please retest this on your live site ?

Optin monster

Submit lead form event tracked when only analytics is connected

image

When only Ad module is connected

image

WP Forms

When only Ads module is connected

image

When only Analytics is connected

image

MailChimp

When only Analytics is connected

image

When only Ads module is connected

image

Popup Maker

When only ads module is connected

image

When only Analytics is connected

image

WooCommerce -

Add to cart event tracked

image

from site-kit-wp.

wpdarren avatar wpdarren commented on August 29, 2024

@mohitwp yes, I can confirm that the source appears for the contact conversion event.

image

from site-kit-wp.

mohitwp avatar mohitwp commented on August 29, 2024

QA Update ✅

Thanks @wpdarren !

  • Tested on dev environment.
  • Verified Optin Monster, WP Forms, MailChimp, Popup Maker, Contact Form 7 and WooCommerce plugin events.
  • Verified that events tracked for WPForms OptIn Monster, Popup Maker, Mailchimp, and WooCommerce providers now tracked with _source: "site-kit".
  • Verified when conversionInfra feature flag is enabled.
  • Verified when Analytics is connected.
  • Verified when both Analytics and ads module are connected.
  • Verified when only Ads module is connected.

Note : As mentioned on #8553 (comment) for WooCommerce only 'add to cart' event is getting tracked.

Optin monster

Submit lead form event tracked when only analytics is connected

image

When only Ad module is connected

image

WP Forms

When only Ads module is connected

image

When only Analytics is connected

image

MailChimp

When only Analytics is connected

image

When only Ads module is connected

image

Popup Maker

When only ads module is connected

image

When only Analytics is connected

image

WooCommerce -

Add to cart event tracked

image

from site-kit-wp.

aaemnnosttv avatar aaemnnosttv commented on August 29, 2024

Noting that the source parameter may change but it is currently implemented as defined.

One thing from the AC

All existing plugin scripts of the listed supported plugins should be updated to be loaded async and include this new script as a dependency

All the provider scripts should be loaded using deferasync scripts can't have dependencies since the load order isn't guaranteed. There are still a few scripts loading async so these will need to be updated in a follow up.

from site-kit-wp.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.