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

Track only specific headings #2328

Merged
merged 1 commit into from
Jan 7, 2022
Merged

Track only specific headings #2328

merged 1 commit into from
Jan 7, 2022

Conversation

andysellick
Copy link
Contributor

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

What

Applies to https://www.gov.uk/guidance/travel-abroad-from-england-during-coronavirus-covid-19 and https://www.gov.uk/guidance/travel-to-england-from-another-country-during-coronavirus-covid-19

  • for the given URLs, only scroll track some of the headings, not all of the headings
  • this will need to be updated in future if the headings change

Why

We only want to scroll track these headings. Anything else is just adding unneeded data.

Visual changes

None.

Trello card: https://trello.com/c/sdugtMbX/37-migrate-to-new-scroll-tracker

@govuk-ci govuk-ci temporarily deployed to government-f-track-spec-jxpnqt January 7, 2022 12:46 Inactive
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Changes look good - have you tried it out on integration? just want to confirm that the HTML output all wires up correctly.

It'd be useful to have some context in the commit message (and PR in general) about why we need to do this complex and fragile approach for these pages? I'm assuming there's a problem of some sort in what you had before and that this is probably a non-ideal compromise?

- for the given URLs, only scroll track some of the headings, not all of the headings
- this will need to be updated in future if the headings change, as this is a fragile approach
- we were tracking all of the headings, but we only want to track the given headings, and there's a lot of other headings on this page which is resulting in a lot of data being sent unnecessarily to Google Analytics
@andysellick andysellick force-pushed the track-specific-headings branch from aca4846 to 49a01ff Compare January 7, 2022 13:42
@govuk-ci govuk-ci temporarily deployed to government-f-track-spec-jxpnqt January 7, 2022 13:43 Inactive
@andysellick
Copy link
Contributor Author

Thanks @kevindew have updated the commit message to add some more detail.

I've just tried it on integration and it seems fine. Not sure what you meant by the HTML output wiring up correctly, but here's the view source, seems to be all in one line and fairly clean.

Screenshot 2022-01-07 at 13 48 25

@kevindew
Copy link
Member

kevindew commented Jan 7, 2022

I've just tried it on integration and it seems fine. Not sure what you meant by the HTML output wiring up correctly, but here's the view source, seems to be all in one line and fairly clean.

Ah I just meant that it all worked as expected.

Thanks for adding the extra detail. Hmm has someone complained about this? It doesn't seem too big a deal to me to be sending unnecessary data to GA - certainly better that than we add extra fragile code where tracking breaks when the content does

Looking at: https://www.gov.uk/guidance/travel-abroad-from-england-during-coronavirus-covid-19 there are 17 headers in main and we're tracking 9 so it's not much of a difference for the overhead. The other one seems to have 30 headers - but still seems a reasonable quantity to track.

@andysellick
Copy link
Contributor Author

I was asked to track only those specific headings because I think although the difference between those tracked and the total number is small, it amounts to quite a lot of unneeded data when you multiply up all the visits - and these are some of our most visited pages at the moment. Agreed though, this is a fragile approach and one I'll try to discourage.

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Cool - well sounds reasonable and I know this is only caught in the midst of a migration and not a new change.

@andysellick andysellick merged commit fa1394e into main Jan 7, 2022
@andysellick andysellick deleted the track-specific-headings branch January 7, 2022 14:16
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