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

Conversation

Projects
None yet
3 participants
@colinrotherham
Contributor

colinrotherham commented Sep 26, 2016

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

This comment has been minimized.

Show comment
Hide comment
@colinrotherham

colinrotherham Sep 27, 2016

Contributor

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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@nickcolley

nickcolley Sep 27, 2016

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

Contributor

nickcolley commented Sep 27, 2016

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

@colinrotherham

This comment has been minimized.

Show comment
Hide comment
@colinrotherham

colinrotherham Sep 27, 2016

Contributor

Thanks @nickcolley.

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@robinwhittleton

robinwhittleton Sep 27, 2016

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?

Contributor

robinwhittleton commented Sep 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
@colinrotherham

colinrotherham Oct 5, 2016

Contributor

Hi @nickcolley,

Do you still plan to merge this in?

Thanks

Contributor

colinrotherham commented Oct 5, 2016

Hi @nickcolley,

Do you still plan to merge this in?

Thanks

@nickcolley

This comment has been minimized.

Show comment
Hide comment
@nickcolley

nickcolley Oct 6, 2016

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

Contributor

nickcolley commented Oct 6, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@robinwhittleton

robinwhittleton Oct 24, 2016

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.

Contributor

robinwhittleton commented Oct 24, 2016

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gemmaleigh gemmaleigh referenced this pull request Oct 25, 2016

Merged

v5.0.0 #346

gemmaleigh added a commit to alphagov/govuk_elements that referenced this pull request Oct 25, 2016

Bump govuk_frontend_toolkit to 5.0.0
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

Upgrade frontend toolkit to version 5.0.1
- 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

Upgrade frontend toolkit to version 5.0.2
- 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