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

Allow custom options in GOVUK.analytics.trackPageview #332

Merged
merged 5 commits into from
Oct 24, 2016
Merged

Allow custom options in GOVUK.analytics.trackPageview #332

merged 5 commits into from
Oct 24, 2016

Conversation

colinrotherham
Copy link
Contributor

Currently the GOVUK.analytics.trackPageview() method only supports the transport 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):

GOVUK.analytics.trackPageview('/', 'Home', {
  sessionControl: 'start'
})

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Sep 27, 2016

Hi @dsingleton, @NickColley,

I've improved the way options can be set on GOVUK.analytics.trackPageview(), would you mind taking a look for me in this pull request?

The optionsparameter (extended by me, but existed already) isn't documented in analytics.md but it would be useful to others. Shall I add this or would you rather keep it as a hidden feature?

@NickColley
Copy link
Contributor

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.. 👍

Copy link
Contributor

@robinwhittleton robinwhittleton left a 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;
Copy link
Contributor

@robinwhittleton robinwhittleton Sep 27, 2016

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'.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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 👍

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Sep 27, 2016

Thanks @NickColley.

Spotted by @robinwhittleton were two places options was being redeclared (both trackEvent() and trackPageview()) which are now sorted.

@robinwhittleton
Copy link
Contributor

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?

@colinrotherham
Copy link
Contributor Author

Hi @NickColley,

Do you still plan to merge this in?

Thanks

@NickColley
Copy link
Contributor

@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..

@robinwhittleton
Copy link
Contributor

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.

@robinwhittleton robinwhittleton merged commit 70865e2 into alphagov:master Oct 24, 2016
@gemmaleigh gemmaleigh mentioned this pull request Oct 25, 2016
gemmaleigh added a commit to alphagov/govuk_elements that referenced this pull request Oct 25, 2016
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))
Guntrisoft added a commit to guidance-guarantee-programme/pension_guidance that referenced this pull request Dec 2, 2016
- 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
Guntrisoft added a commit to guidance-guarantee-programme/pension_guidance that referenced this pull request Dec 2, 2016
- 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
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.

3 participants