-
Notifications
You must be signed in to change notification settings - Fork 153
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 limited GA to the buyers guide #1974
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.
This looks like a reasonable start- I'm fine landing it to get some sort of GA in there.
|
||
let productLiveChat = document.querySelector(`#product-live-chat`); | ||
let siteSocialButtonFacebook = document.querySelector(`#nav-social-button-fb`); |
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.
Might as well use document.getElementById
if we're using it that much, since it's considerably faster than querySelector
.
}; | ||
} | ||
|
||
if (siteSocialButtonFacebook) { |
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.
If we're repeating code this much, it's probably better to make this a single action, and then trigger them for each element:
function addAnalytics(elementId, category, action, label, beacon) {
let element = document.getElementById(elementId);
if (element) {
let opts = { category, action, label };
if (beacon) { opts.transport = `beacon`; }
element.onclick = () => ReactGA.event(opts); // <= this is... probably bad?
} else {
// we can warn about the fact that an expected element does not exist, here
}
}
...
addAnalytics(`nav-social-button-fb`, `site`, `share tap`, `share site to facebook`, true);
// etc
Note the "this is probably bad", though, because onclick
is a plain property: if something else sets an onclick first, that is now lost, and if something else sets an onclick later, that will remove the GA binding, so ideally we use addEventListener
...
<a class="social-button social-button-fb hidden-sm-down" id="nav-social-button-fb" href="{{facebook_share_link}}"><span class="sr-only">Facebook</span></a> | ||
<a class="social-button social-button-twitter hidden-sm-down" id="nav-social-button-twitter" href="{{twitter_share_link}}{{share_text|urlencode}}" target="_blank"><span class="sr-only">Twitter</span></a> | ||
<a class="social-button social-button-email hidden-sm-down" id="nav-social-button-email" href="{{email_share_link}}{{share_text|urlencode}}"><span class="sr-only">Email</span></a> | ||
<a class="social-button social-button-link copy-link hidden-sm-down" id="nave-social-button-link" href=""><span class="sr-only">Copy link</span></a> |
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.
nave
typo?
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.
Are we tracking clicks on mobile as well? If so let's add id
to elements on mobile version.
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.
👍 let me add those
|
||
function getElementGAInformation(pageTitle, productName) { | ||
return { | ||
'product-live-chat`': { |
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.
extra ` here
Addresses some of the events in #1925, but not done in a real sense.
Added:
note that bg-main.js does not import and init
product-analytics.js
yet