-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor google analytics to use gtag #911
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.
Overall this looks great. It's nice that we don't have to use the wrapper components anymore. I left a couple sanity-check questions to verify before approval.
lib/gtag.js
Outdated
export const event = gaEvent => { | ||
window.gtag("event", gaEvent.contributor, { | ||
category: `${gaEvent.type} : ${gaEvent.partner}`, | ||
label: `${gaEvent.itemId} : ${gaEvent.title}` |
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.
In the docs/examples you link to in the PR, the keys are event_category
and event_label
rather than category
and label
. I'm guessing this makes a difference?
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.
correct. changed (re-sanity check?). i notice you don't use the value
and assume that's fine
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.
That is correct, I don't use the value
field.
components/shared/ListView/index.js
Outdated
const links = document.getElementsByClassName("internalItemLink"); | ||
const items = this.props.items; | ||
|
||
// alert(`${JSON.stringify(items[0].linkAs)} : ${JSON.stringify(links[0].href)}`); |
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.
I'm guessing this is leftover from some debugging task and can be deleted.
window.dataLayer = window.dataLayer || []; | ||
function gtag(){dataLayer.push(arguments);} | ||
gtag('js', new Date()); | ||
gtag('config', '${gaTrackingId}'); |
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.
I don't recall Document
or MyDocument
is used so just to sanity check, can you clarify when exactly this gtag('config', '${gaTrackingId}')
pageview tracking event would be fired? Is it just when MainLayout
is rendered?
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.
this is the wrapper of any/every page with or without MainLayout
. this should instantiate gtag
on any server-side page render. client-side calls would not re-instantiate.
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.
I am admittedly a little fuzzy about how the pageview stuff executes, but let's test it on staging.
per vercel/next.js#4036,
gtag.js
replaces olderanalytics.js
solution and is currently recommended by Google - https://developers.google.com/analytics/devguides/collection/gtagjs/migrationthe previous solution relied on wrapper components that got in the way and were very redundant. those components were removed and the analytics were implemented as recommended in the nextjs
with-google-analytics
example: https://github.com/zeit/next.js/tree/master/examples/with-google-analyticsthe
lib/googleAnalytics
folder was also removed and replaced with alib/gtag.js
file. addingalert
s to that file should allow for testingfixes #907