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

Refactor GA4 analytics event and link trackers #3057

Merged
merged 10 commits into from
Nov 25, 2022
Merged

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Nov 8, 2022

What

Restructure our code for link and event tracking in Google Analytics 4, making the following changes:

Why

We want to clarify both the intent and the code structure for link and event tracking in different situations. Despite the overlap, it feels sensible to have specific scripts for tracking interactive elements like buttons (event tracking), regular links (link tracking) and other kinds of links (specialist link tracking).

Event tracking and link tracking are applied manually to elements using data attributes. Specialist link tracking acts automatically.

How to upgrade this breaking change

  • all uses of data-module="ga4-event-tracker" must rename all their attributes and child attributes that are currently data-ga4 to data-ga4-event
  • instances of the share links component that have GA4 tracking implemented on them might need checking

Visual Changes

None.

Trello cards:

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 09:47 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 10:47 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 11:28 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 11:38 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 11:40 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 11:42 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 11:56 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 13:29 Inactive
@andysellick andysellick marked this pull request as ready for review November 8, 2022 13:57
@andysellick andysellick changed the title [DO NOT MERGE] Refactor GA4 analytics event and link trackers Refactor GA4 analytics event and link trackers Nov 8, 2022
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 8, 2022 13:57 Inactive
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @andysellick - There's some cleanup to do, and I think method needs to be calculated via JavaScript in Ga4LinkTracker.

There's also a question on wether it should be possible for external to be calculated dynamically in Ga4LinkTracker. This is because there are some external links we have which would use Ga4LinkTracker. For example, I looked in our implementation guide and can see:

  • Footer link tracking (our footer has links to nationalarchives.gov.uk),
  • 'Banner link clicked' which can go to surveys
  • 'County, district or unitary council link' which will go to external domains
  • The share/follow links which go to social media sites

I understand that external can be hardcoded into each data-ga4-link JSON in most cases, but if a link is being pulled in via content which we can't directly access, then I guess JavaScript will need to determine external on click. Also not sure how tedious it would be to ensure every link is external/internal if we are manually adding that to each component's links.

It looks like the Share/Follow links should be moved to use Ga4LinkTracker too, and as we discussed maybe the accordions should be moved to use a ga4_tracking in their template so that we can remove tech debt without another breaking change (unless we do this in a separate branch.)

Thanks

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 15, 2022 12:08 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 15, 2022 16:31 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 16, 2022 09:13 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 16, 2022 09:18 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 16, 2022 09:52 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 23, 2022 09:26 Inactive
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

Looks good to me now, hopefully we haven't missed anything. I'm guessing the commits will be squashed? Thanks for doing this as it was a lot of work 👍

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 24, 2022 09:00 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 24, 2022 09:03 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 24, 2022 09:04 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 24, 2022 09:06 Inactive
- to allow tracking to be manually added to links on any page
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 24, 2022 09:08 Inactive
- reduce some code repetition by moving a few functions in ga4-core
- remove disablelisteners option from specialist link tracker, as not in use
- use a regex to simplify hrefPointsToDomain, which determines if a link href attribute refers to a specific domain
- share the text cleaning code between the link tracker and the specialist link tracker, ensures that any link text is clean from extra spaces or newlines
- streamline data attribute handling, as code was being duplicated unnecessarily
- move removeCrossDomainParams to the core functions so can be shared by both link trackers
- prevent link and event tracker from clashing, by distinctly renaming the attributes they depend on from 'data-ga4' to 'data-ga4-link' and 'data-ga4-event' respectively (BREAKING CHANGE)
- add tests for functions now included for link tracking in core functions
- switch the share links component over to using the ga4-link-tracker to track it instead of the specialist link tracker
- therefore remove some code from the specialist tracker and add a check for the ga4 link tracker's attributes, to stop it tracking those
- move the code for including the domain of a link into the core functions, so both the specialist link checker and the link checker can both use it
- add tests
- update documentation to reflect new changes
- update other bits of code that rely on the event tracker, where data-ga4 attributes must now be data-ga4-event
- removing entirely from feedback-spec.js as not needed for the tests to run
- text of the tracked link can now be overridden in the link tracker by providing a value for text in the associated data attribute
- update the share link component to make use of this (well, not exactly, just remove the explicit declaration for this in the track_as_follow option where no longer needed)
- update documentation for the share links component to highlight the visually hidden text, and update examples to fix incorrect text
- also reorder share links documentation to introduce custom hidden text earlier and reprioritise other examples
- move appendDomainsWithoutWWW into core functions
- change how internalDomains are stored from being part of the specialist link tracker to being one of the window.GOVUK.analyticsGa4.vars
- initialise www.gov.uk on analytics load in .vars and call appendDomainsWithoutWWW then
- update references and tests relying on internalDomains
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3057 November 24, 2022 09:10 Inactive
@andysellick
Copy link
Contributor Author

Thanks @AshGDS. I've squashed it down from 19 commits to 10, which feels more manageable, although some of them have got fairly big and I started to hit merge conflicts so I think I'll leave it at that.

Since this is a breaking change I'll hold off merging it until we're ready to implement the breaking changes across applications.

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