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
Track Related Item clicks #875
Conversation
Please review, but don't merge yet - I'm going to run this past @carvil when he's back on Monday. |
I'm not that well versed with the Analytics code yet but it seems from first glance that we could add the dimensions to the already existing I'll add @fofr as a reviewer :) |
dimension = element.data('track-dimension'), | ||
trackClick = new GOVUK.Modules.TrackClick(); | ||
|
||
trackClick.start(element); | ||
|
||
function trackBreadcrumbClick(e) { | ||
function onClick(e) { |
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 method name should be descriptive of what the function does, not the circumstance that it's called in
I agree with @NickColley, I don't see the need for both |
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.
Looks good, only a couple of tweaks. Please see below.
category = element.attr('data-track-category'), | ||
action = element.attr('data-track-action'), | ||
label = element.attr('data-track-label'); | ||
category = element.data('track-category'), |
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 think we should use attr()
instead of data()
when fetching data from these data attributes, more details here: ebe69aa#diff-5649f4c1766856649d723e2ef9a026b3
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.
Will do. Although the dimension has always been a number rather than a string - should we keep it that way? (Ie use element.data('track-custom-dimension')
where all the others are .attr
?
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.
As discussed, custom dimension should remain data because Google Analytics expects an integer.
@@ -14,7 +14,8 @@ | |||
track_action: index + 1, | |||
track_label: crumb[:url], | |||
track_dimension: crumb[:title], | |||
module: 'track-breadcrumb-click' | |||
track_custom_dimension: 29, |
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.
We could perhaps set this in a static variable with a name that explains when this is in case people wonder what the number 29 is all about. Something like:
LINK_TEXT_CUSTOM_DIMENSION=29
And then use it throughout. What do you think?
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.
Sounds good to me - where should it live?
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.
As discussed, will leave this for now.
); | ||
}); | ||
|
||
it('tracks related item click events', function() { |
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.
Could you please also add a test that makes sure we call GOVUK.analytics.trackEvent
but don't call GOVUK.analytics.setDimension
when the custom dimension attribute isn't present?
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.
Aren't the changes to 'tracks click events'
sufficient? (lines 11 & 28). Or did you want it to be more explicit than that?
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.
As discussed, this is fine.
f19f1f3
to
f01d1a6
Compare
} | ||
|
||
if (customDimension && GOVUK.analytics.setDimension) { | ||
GOVUK.analytics.setDimension(customDimension, dimension); |
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 dimension
is not set, could this break?
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've added a check and some tests for this.
1727b78
to
bc02a6f
Compare
label = element.attr('data-track-label'); | ||
label = element.attr('data-track-label'), | ||
dimension = element.attr('data-track-dimension'), | ||
customDimension = element.data('track-custom-dimension'); |
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 these two variables the custom dimension index and the custom dimension value?
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.
Yes I believe so - I can rename if you want? (I'm still only just getting my head around everything!)
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 think it'd be clearer if renamed
} | ||
|
||
if (customDimension && dimension && GOVUK.analytics.setDimension) { | ||
GOVUK.analytics.setDimension(customDimension, dimension); |
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 think this dimension will remain set for all subsequent tracked-clicks.
eg If I cmd+click and open a page in a new tab. Then on the old page if I click an external link or a download – (two types of links that get tracked), those will be tracked with this dimension.
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.
Very possible - I have no idea how this is put together. How do I "unset" it? Set it to undefined?
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 guess technically for compatibility it should be set to null
?)
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.
Rather than setting the dimension I think we should pass it as an option into the event itself:
And:
https://developers.google.com/analytics/devguides/collection/analyticsjs/custom-dims-mets
eg
GOVUK.analytics.trackEvent(category, action, {dimension29: "dimensionValue"});
That should work.
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.
As a side-note, I'm not sure we do track downloads? For example the PDF on the Computing Curriculum page doesn't have any tracking markup, and I don't see any events fired off to GA in my Network tab. Unless it's a specific type of download?
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.
Similarly, I don't see any tracking markup on the link to the DHS Website on this page? Are you aware of an actual case I can test?
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.
(The above solution looks good to me, but I'd like to confirm on a test case if you can find one?)
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's configured here:
static/app/assets/javascripts/analytics/static-analytics.js
Lines 26 to 28 in 9e8795b
GOVUK.analyticsPlugins.downloadLinkTracker({ | |
selector: 'a[href*="/government/uploads"], a[href*="assets.publishing.service.gov.uk"]' | |
}); |
Though you're right, I can't see anything being tracked.
I see the requests in my network tab correctly (my console had filtered to show only img requests, switching to all showed the correct events being sent through in a request payload)
This pull request is now dependent upon the deployment of alphagov/govuk_frontend_toolkit#365 |
This reverts commit 50517980434ed10aa583843d4b90fd82c44e7aa8.
c3c36f3
to
8c20044
Compare
5759edc
to
03e3469
Compare
03e3469
to
e5831a2
Compare
@fofr @NickColley this should now be good to go. It was waiting on the Frontend Toolkit Gem getting bumped (which it now has). I've tested this change end-to-end with my changes to the Frontend Toolkit Gem. |
I've tried it in integration and I can see the GA events being fired correctly. 👍 |
This change will add tracking for clicks on the "Related Item" links.
Trello card: https://trello.com/c/ybdOyNSu/288-enable-tracking-of-related-links-clicks-across-all-gov-uk-applications
The change is almost a direct read-across of the same change for Breadcrumbs:
Note that we are reusing the "Link Text" Custom Dimension 29 (See: https://gov-uk.atlassian.net/wiki/display/GOVUK/Analytics+on+GOV.UK#AnalyticsonGOV.UK-customDimensionsCustomdimensions&suk=ff808081595cb8c4015963b402330000)