From 8ed4848b8fcf10ae6aab90517d5306b8858dc7bc Mon Sep 17 00:00:00 2001 From: JonathanHallam Date: Tue, 26 Oct 2021 14:15:23 +0100 Subject: [PATCH 1/2] remove jq from ga tracker and application.js The only code in application.js is to initialise the ga tracker so they're being done at the same time. I've removed one `get` but not the other because the `tracker` object is defined by google while the `form` object is a standard HTML element You will notice that the application.js calls on the `getElementsByClassName` method which can return multiple results. Then we're only using the first element of that list. This isn't ideal but we have confidence that this class can only be used once on each page which makes this fine. Call route for the set-ga-client-id-on-form - application.js is the only file that calls on it - it only applies it to elements with the `js-service-sign-in-form` class - that class is only created by [this partial](https://github.com/alphagov/government-frontend/blob/main/app/views/content_items/service_sign_in/_choose_sign_in.html.erb#L25) - that partial is only rendered by [this page](https://github.com/alphagov/government-frontend/blob/main/app/views/content_items/service_sign_in.html.erb#L8) by way of a method in [this presenter](https://github.com/alphagov/government-frontend/blob/main/app/presenters/service_sign_in/choose_sign_in_presenter.rb#L5) - This means we can be confident there's only one element of that class on a page, huzah! --- app/assets/javascripts/application.js | 10 ++++------ app/assets/javascripts/set-ga-client-id-on-form.js | 10 ++++------ spec/javascripts/set-ga-client-id-on-form.spec.js | 2 +- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 9cf647db4..fc6dece59 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -12,10 +12,8 @@ //= require_tree ./modules //= require set-ga-client-id-on-form -jQuery(function ($) { - var $form = $('.js-service-sign-in-form') +var form = document.querySelector('.js-service-sign-in-form') - if ($form.length) { - new GOVUK.SetGaClientIdOnForm({ $form: $form }) // eslint-disable-line no-new - } -}) +if (form) { + new GOVUK.SetGaClientIdOnForm(form) // eslint-disable-line no-new +} diff --git a/app/assets/javascripts/set-ga-client-id-on-form.js b/app/assets/javascripts/set-ga-client-id-on-form.js index 7998804eb..dd61d789b 100644 --- a/app/assets/javascripts/set-ga-client-id-on-form.js +++ b/app/assets/javascripts/set-ga-client-id-on-form.js @@ -4,15 +4,13 @@ window.GOVUK = window.GOVUK || {} var GOVUK = window.GOVUK - function SetGaClientIdOnForm (options) { - if (!options.$form || !window.ga) { return } - - var form = options.$form + function SetGaClientIdOnForm (form) { + if (!form || !window.ga) { return } window.ga(function (tracker) { var clientId = tracker.get('clientId') - var action = form.attr('action') - form.attr('action', action + '?_ga=' + clientId) + var action = form.getAttribute('action') + form.setAttribute('action', action + '?_ga=' + clientId) }) } diff --git a/spec/javascripts/set-ga-client-id-on-form.spec.js b/spec/javascripts/set-ga-client-id-on-form.spec.js index c4a605463..937d927e8 100644 --- a/spec/javascripts/set-ga-client-id-on-form.spec.js +++ b/spec/javascripts/set-ga-client-id-on-form.spec.js @@ -11,7 +11,7 @@ describe('SetGaClientIdOnForm', function () { form = $( '
' ) - new GOVUK.SetGaClientIdOnForm({ $form: form }) // eslint-disable-line no-new + new GOVUK.SetGaClientIdOnForm(form[0]) // eslint-disable-line no-new }) it('sets the _ga client id as a query param on the form action', function () { From 3d40e4134b758810300b41f0da7671d9de3434f4 Mon Sep 17 00:00:00 2001 From: JonathanHallam Date: Thu, 28 Oct 2021 12:11:46 +0100 Subject: [PATCH 2/2] Rename tracker in tests tracker is an object that comes directly from google. This is why the `get` method call has been kept on it, it's not a jQ object so the method is defined by google rather than jQ. I've renamed the `tracker` object in the tests to make it more obvious that it's something we don't have ownership over --- spec/javascripts/set-ga-client-id-on-form.spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/javascripts/set-ga-client-id-on-form.spec.js b/spec/javascripts/set-ga-client-id-on-form.spec.js index 937d927e8..b6c6c8718 100644 --- a/spec/javascripts/set-ga-client-id-on-form.spec.js +++ b/spec/javascripts/set-ga-client-id-on-form.spec.js @@ -2,9 +2,9 @@ var $ = window.jQuery describe('SetGaClientIdOnForm', function () { var GOVUK = window.GOVUK - var tracker = { clientId: 'clientId' } - tracker.get = function (arg) { return this[arg] } - window.ga = function (callback) { callback(tracker) } + var mockTracker = { clientId: 'clientId' } + mockTracker.get = function (arg) { return this[arg] } + window.ga = function (callback) { callback(mockTracker) } var form beforeEach(function () {