Skip to content
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 support for GA Universal #421

Merged
merged 5 commits into from
Sep 26, 2018
Merged

Add support for GA Universal #421

merged 5 commits into from
Sep 26, 2018

Conversation

gunjam
Copy link
Contributor

@gunjam gunjam commented Jun 26, 2017

Google Analytics Universal provides a function ga that sends all
communication to Google Analytics. The sendGoogleAnalyticsEvent
function now checks if the ga function exists and if it does, uses it.

Port of commit 6bb47fd by @robyoung

@NickColley
Copy link
Contributor

I noticed in the previous implementation there's some tests, is it possible to get this tested here?

@gunjam
Copy link
Contributor Author

gunjam commented Jun 27, 2017

I've added in the missing unit tests from the previous version.

Ta.

@robyoung
Copy link

robyoung commented Jul 4, 2017

Wow! @gunjam I am super impressed. How did you find that commit? I'd forgetten I even worked on that project.

@NickColley
Copy link
Contributor

This looks good to me, but I think will be a breaking change since it'll always favour the ga global (which should be fine but is a breaking behaviour) 👍

@rpowis
Copy link

rpowis commented Feb 14, 2018

Hi all. Is there any chance of getting this merged? Some of our services have updated to universal analytics and I'd love to get rid of our fork of the old repo.

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a changelog entry and approved this.

Will need another approval from the team before this can be merged.

@NickColley
Copy link
Contributor

Going to ping the Analytics community at GDS to see if they're OK with this change.

@@ -62,7 +62,11 @@
}())

GOVUK.performance.sendGoogleAnalyticsEvent = function (category, event, label) {
global._gaq.push(['_trackEvent', category, event, label, undefined, true])
if (global.ga && typeof global.ga === 'function') {
global.ga('send', 'event', category, event, label)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sixth element in the trackEvent array means means that opt_noninteraction is true, which means that the 'event hit will not be used in bounce-rate calculation'.

Assuming we'd want the same behaviour with GA Universal, I think we'd want to make this:

global.ga('send', 'event', category, event, label, {
  nonInteraction: true
})

as per https://developers.google.com/analytics/devguides/collection/analyticsjs/events#non_interaction_events?

I'm not a performance analyst however… there may be other better ways of achieving the same thing without hardcoding it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems sensible to mirror them.

@rpowis would be good to get your thoughts on this.

Copy link

@rpowis rpowis Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good spot @36degrees. i think this got overlooked for whatever reason in our forked version.

so as things stand, any page using universal tracking with an event on it has been adding to the bounce rate if that event hasn't been triggered? 😬

similar to the _gaq call, we probably don't want to add to the bounce rate in the majority of cases so i'd say let's default to { nonInteraction: true }.

having the ability to opt-in would be good but i don't think any services here are getting that granular with their analytics so i'm not sure it would need to be done as part of this PR.

@NickColley
Copy link
Contributor

NickColley commented Sep 14, 2018

This doesnt seem to be used much internally at GDS, but rather a library used for external services: https://github.com/search?l=JavaScript&p=1&q=org%3Aalphagov+GOVUK.performance&type=Code

Update: I went through these and could not find any up to date usage of this.

Google Analytics Universal provides a function `ga` that sends all
communication to Google Analytics. The `sendGoogleAnalyticsEvent`
function now checks if the `ga` function exists and if it does, uses it.

Port of commit 6bb47fd (https://git.io/vQs2t) by @robyoung
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but a couple of suggestions for the changelog entry.

CHANGELOG.md Outdated
@@ -1,6 +1,13 @@
# Unreleased

- Fix: Add support to show-hide-content.js for form fields with more varied characters: ([PR 403](https://github.com/alphagov/govuk_frontend_toolkit/pull/403))
- Breaking change: Add support for GA Universal in stageprompt.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we explain why this is a breaking change, and what users need to do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good shout will update

- Fix: Add support to show-hide-content.js for form fields with more varied characters: ([PR 403](https://github.com/alphagov/govuk_frontend_toolkit/pull/403))
- Breaking change: Add support for GA Universal in stageprompt.js

Also ensures that events triggered via GA Universal do not impact bounce-rate calculations,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need mentioning, as it's not a change from anything that's been released?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we know of forks that do not include this, I think it's worth mentioning.

NickColley and others added 2 commits September 26, 2018 13:15
This matches the previous analytics track event function.

From @36degrees:

The sixth element in the trackEvent array means means that opt_noninteraction is true, which means that the 'event hit will not be used in bounce-rate calculation'.

as per https://developers.google.com/analytics/devguides/collection/analyticsjs/events#non_interaction_events
@NickColley
Copy link
Contributor

I have spoken to @gunjam and got their approval for the updates I have made.

@36degrees
Copy link
Contributor

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants