-
Notifications
You must be signed in to change notification settings - Fork 20
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
Re-work explicit-cross-domain-links.js
#2502
Re-work explicit-cross-domain-links.js
#2502
Conversation
explicit-cross-domain-links.js
96222e2
to
0d5341c
Compare
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, couple of small comments but happy to approve.
@@ -6,78 +6,79 @@ | |||
|
|||
GOVUK.Modules.ExplicitCrossDomainLinks = function () { | |||
this.start = function ($module) { |
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.
While you're at it... 😁
It seems like this module doesn't contain any jQuery, so you could switch it from using a start
function to an init
function. Shouldn't be a huge change - you'll need to change line 9 to this.element = $module
and update how the code is called in the test, but that should be about it.
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.
Good suggestion, but do you mind if we do this as a separate piece of work as it's actually not so straightforward? Happy to record it as an issue in the meanwhile
app/assets/javascripts/govuk_publishing_components/analytics/explicit-cross-domain-links.js
Outdated
Show resolved
Hide resolved
0d5341c
to
5e83b41
Compare
It has transpired that the explicit-cross-domain-links.js script was not set up correctly which is breaking our cross domain tracking currently. The source of the issue is the ga parameter which contains a timestamp which actually expires and becomes invalid after two minutes. The way the script worked previously was that the relevant cross-domain links and forms on the page were decorated with the _ga and cookie_consent parameters on page load. Doing it on page load is an issue due to the time-sensitive nature of the _ga parameter, as mentioned above. This changes the way the script works so that instead of decorating crossdomain links and forms with additional parameters on page load, we decorate them when they are interacted with. Besides ensuring that the _ga param is always valid and up-to-date, this also makes it so that there is no longer a need to listen for cookie banner events, so I also removed the cookie-reject event as part of this commit.
5e83b41
to
3684c2d
Compare
It has transpired that the
explicit-cross-domain-links.js
script was notset up correctly which is breaking our cross domain tracking currently.
The source of the issue is the
_ga
parameter which contains a timestampwhich actually expires and becomes invalid after two minutes.
The way the script worked previously was that the relevant cross-domain
links and forms on the page were decorated with the
_ga
andcookie_consent
parameters on page load. Doing it on page load is anissue due to the time-sensitive nature of the
_ga
parameter, asmentioned above.
This changes the way the script works so that instead of decorating
crossdomain links and forms with additional parameters on page load, we
decorate them when they are interacted with.
Besides ensuring that the
_ga
param is always valid and up-to-date, thisalso makes it so that there is no longer a need to listen for cookie
banner events, so I also removed the
cookie-reject
event as part of thiscommit (this event was explicitly introduced for the
explicit-cross-domain-links.js
script and is not used anywhere else currently).
https://trello.com/c/VaRq2typ/27-fix-cross-domain-issue