-
Notifications
You must be signed in to change notification settings - Fork 104
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
Allow custom options in GOVUK.analytics.trackPageview #332
Allow custom options in GOVUK.analytics.trackPageview #332
Conversation
Hi @dsingleton, @NickColley, I've improved the way options can be set on The |
Hey @colinrotherham Thanks for the PR! I had a look over the code it looks good, I think it'd be good to add the note to the docs if you have time.. 👍 |
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.
Minor nit
@@ -33,24 +34,25 @@ | |||
|
|||
// https://developers.google.com/analytics/devguides/collection/analyticsjs/pages | |||
GoogleAnalyticsUniversalTracker.prototype.trackPageview = function(path, title, options) { | |||
var options = options || {}; | |||
var options; |
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.
Don’t think we need this line right? options
is coming in as an argument and if it’s not supplied if (typeof options === "object")
will just fail as options
will equal '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.
Good point. I saw it as a simplification, but missed that you were re-declaring options
.
Let's tidy this up.
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 trackEvent
method was doing the same.
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 would be caught when we add linting 👍
Thanks @NickColley. Spotted by @robinwhittleton were two places |
So this looks conceptually fine to me, but I haven’t tested it on an actual site and I’m not going to have time to this week. @NickColley, is this something you could do? |
Hi @NickColley, Do you still plan to merge this in? Thanks |
@colinrotherham I haven't had the time to test this sorry Colin will try to ASAP but hopefully someone else can review, will ping others too.. |
E.g. Force a session reset with { sessionControl: 'start' }
Merge in changes from master
Tested this with a few different inputs and it’s working as expected, so I’m going to merge. It would be good to get this tested with analytics people on GOV.UK before static is updated to include it. |
This release includes two breaking changes: - Removal of external link styles and icons, if you are using the external-link-* mixins you will need to remove them from your codebase ([PR #293](alphagov/govuk_frontend_toolkit#293)) - Correct spelling of the 'accordion' icon, you will need to check for the incorrect spelling 'accordian' and update if you are using this icon ([PR #345](alphagov/govuk_frontend_toolkit#345)) And two minor changes: - Amend GOVUK.StickAtTopWhenScrolling to resize the sticky element and shim when the .js-sticky-resize class is set ([PR #343](alphagov/govuk_frontend_toolkit#343)) - Allow custom options in GOVUK.analytics.trackPageview ([#332](alphagov/govuk_frontend_toolkit#332))
- Removed phase-banner scss file as no longer needed Full list of changes: SelectionButtons will add a class to the label with the type of the child input alphagov/govuk_frontend_toolkit#317 Add GOVUK.ShowHideContent JavaScript to support showing and hiding content, toggled by radio buttons and checkboxes alphagov/govuk_frontend_toolkit#315 Fix error in IE - remove trailing comma from shimLinksWithButtonRole JavaScript alphagov/govuk_frontend_toolkit#323 Remove unnecessary print font fallback that causes regression downstream alphagov/govuk_frontend_toolkit#328 Update tooling to use npm script rather than grunt-shell alphagov/govuk_frontend_toolkit#327 For smaller screens (<768px) ensure that the GOVUK.StickAtTopWhenScrolling JS "unsticks" the element which was previously "stuck" (by removing both the class which sets fixed positioning and the shim). alphagov/govuk_frontend_toolkit#329 Lint codebase using standard alphagov/govuk_frontend_toolkit#334 Add semicolons at the start of IIFE's alphagov/govuk_frontend_toolkit#339 Removal of external link styles and icons, if you are using the external-link-* mixins you will need to remove them from your codebase alphagov/govuk_frontend_toolkit#293 Correct spelling of the 'accordion' icon, you will need to check for the incorrect spelling 'accordian' and update if you are using this icon alphagov/govuk_frontend_toolkit#345 Amend GOVUK.StickAtTopWhenScrolling to resize the sticky element and shim when the .js-sticky-resize class is set alphagov/govuk_frontend_toolkit#343 Allow custom options in GOVUK.analytics.trackPageview alphagov/govuk_frontend_toolkit#332 Fix role="button" click shim alphagov/govuk_frontend_toolkit#347 Make font variables lowercase alphagov/govuk_frontend_toolkit#348 Update selection button documentation alphagov/govuk_frontend_toolkit#350 Change colour used in phase tags to govuk-blue alphagov/govuk_frontend_toolkit#353
- Removed phase-banner scss file as no longer needed Full list of changes: SelectionButtons will add a class to the label with the type of the child input alphagov/govuk_frontend_toolkit#317 Add GOVUK.ShowHideContent JavaScript to support showing and hiding content, toggled by radio buttons and checkboxes alphagov/govuk_frontend_toolkit#315 Fix error in IE - remove trailing comma from shimLinksWithButtonRole JavaScript alphagov/govuk_frontend_toolkit#323 Remove unnecessary print font fallback that causes regression downstream alphagov/govuk_frontend_toolkit#328 Update tooling to use npm script rather than grunt-shell alphagov/govuk_frontend_toolkit#327 For smaller screens (<768px) ensure that the GOVUK.StickAtTopWhenScrolling JS "unsticks" the element which was previously "stuck" (by removing both the class which sets fixed positioning and the shim). alphagov/govuk_frontend_toolkit#329 Lint codebase using standard alphagov/govuk_frontend_toolkit#334 Add semicolons at the start of IIFE's alphagov/govuk_frontend_toolkit#339 Removal of external link styles and icons, if you are using the external-link-* mixins you will need to remove them from your codebase alphagov/govuk_frontend_toolkit#293 Correct spelling of the 'accordion' icon, you will need to check for the incorrect spelling 'accordian' and update if you are using this icon alphagov/govuk_frontend_toolkit#345 Amend GOVUK.StickAtTopWhenScrolling to resize the sticky element and shim when the .js-sticky-resize class is set alphagov/govuk_frontend_toolkit#343 Allow custom options in GOVUK.analytics.trackPageview alphagov/govuk_frontend_toolkit#332 Fix role="button" click shim alphagov/govuk_frontend_toolkit#347 Make font variables lowercase alphagov/govuk_frontend_toolkit#348 Update selection button documentation alphagov/govuk_frontend_toolkit#350 Change colour used in phase tags to govuk-blue alphagov/govuk_frontend_toolkit#353
Currently the
GOVUK.analytics.trackPageview()
method only supports thetransport
Google Analytics fieldsObject property, which puts a limit on its usefulness.This limitation makes the following overrides unavailable:
hitCallback
sessionControl
This pull request allows any options object to be used.
For example, the current session can now be reset when returning to a specific page (perhaps when a kiosk session times out):