-
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 new actions for tracking #264
Conversation
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.
Hey @puntope, thanks for your work on modernising the tracking!
This PR is working as expected and does a really nice job of replacing the core tracking but all the events are following the structure for Universal Analytics instead of GA4.
As UA will be retired on the 1st of July 2023 it seems like these changes should be made to support GA4.
I've left a couple of comments but haven't gone through each event as it looks like they will all need to be adjusted.
return; | ||
} | ||
|
||
trackEvent( step === 0 ? 'begin_checkout' : 'checkout_progress', { |
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.
GA4 drops checkout_progress
completely and doesn't include checkout_step
option
listName = __( 'Product List', 'woocommerce-google-analytics-integration' ), | ||
} ) => { | ||
trackEvent( 'view_item_list', { | ||
event_category: 'engagement', |
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.
GA4 drops the event_category
and event_label
structure in favor of individual parameters for each event. Ref: https://developers.google.com/analytics/devguides/collection/ga4/reference/events?client_type=gtag#view_item_list
), | ||
items: products.map( ( product, index ) => ( { | ||
...getProductImpressionObject( product, listName ), | ||
list_position: index + 1, |
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.
Again a UA/GA4 issue and I know it wasn't introduced in this PR but getProductImpressionObject
parameter names have things like id
and name
instead of item_id
and item_name
.
For the map here, list_position
would become index
.
Hello @martynmjones Thanks for the GA changes catch. Since this PR is not going to be merged into develop branch (yet) I will discuss it with the WC Blocks team and perform the updates in other PR if that's ok. In regards to your comment about the missed key, it's already clarified |
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.
Hey @puntope,
Thanks for the clarification 👍
Changes proposed in this Pull Request:
Closes part of #227
This PR implements a set of new actions and their respective tracking functions for handling GA events with
addAction
hook.Replace this with a good description of your changes & reasoning.
Checks:
Detailed test instructions:
console.log(
Tracking event ${ eventName });
)List of doActions.
Additional details:
Changelog entry