OriginProtocol / website-frontend

originprotocol.com front-end
6 stars 6 forks source link

add gtm #87

Closed rolandpo closed 1 year ago

rolandpo commented 1 year ago

add google tag manager code, issue #75 also remove previously added google analytics code for consistency

jonathansnow commented 1 year ago

@rolandpo, this looks good overall (though we will need to set the GTM ID environment variable. I will add it here once configured).

A few questions, though. I'm working backwards on the old analytics code:

I want to ensure there isn't another reason for this user util file outside of Google Analytics before we deploy this code.

The next steps on my end are to finish configuring the GTM container for op.com with both the existing UA pixel and the new GA4 pixel.

rolandpo commented 1 year ago

I cleaned up the remaining packages etc. The previous GA code is what's been used for the OUSD dapp. Since I'm not sure it's even been set up on these new websites, I assume it's best to start from scratch. I believe the User file uses an extra abstraction library on top of GA to identify visitors by utm params. I guess that made it simpler to use both GA and Mixpanel in the dapp, although I'm not sure that Mixpanel is still being actively used, definitely not on these new sites

jonathansnow commented 1 year ago

Thanks for these updates and the explanation. GA is definitely firing on this site, but by using the tested code from story.xyz we should be able to merge without any analytics downtime.

I want to note a comment from the old implementation in the event we need to reference this in the future: There is this weird behaviour with react router where 'routeChangeComplete' gets triggered on initial load only if the URL contains search parameters. And without this check and search parameters present, the initial page view would be tracked twice.

I am seeing accurate firing of the first visit pageview event in GA4 without duplicate events being emitted (on story.xyz).

This looks good to go to me. Before deploying, we'll need to set _NEXT_PUBLIC_GOOGLE_TAG_MANAGER_ID in the environment variables to GTM-WHGCK7S_.

The container is set up, so we are likely good to merge/deploy.

@franckc, not sure if approving this PR auto-merges, but looks good to me. I've not tested this locally, so it might be good to have someone else's eyes on it.

franckc commented 1 year ago

Nice work @rolandpo - I'm going to merge this then will deploy to staging.originprotocol.com after having set the NEXT_PUBLIC_GOOGLE_TAG_MANAGER_ID env var.

Then let's validate it works as expected before deploying to production.