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

Add GA4 tracking to the document list component #3874

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

AshGDS
Copy link
Contributor

@AshGDS AshGDS commented Feb 14, 2024

What

  • Adds GA4 tracking to the document list component
  • Allows the values in the GA4 JSON to be expanded by passing through a ga4_data object when rendering the component, similar to Expand GA4 share link tracking to allow for extra values #3872
  • Note that we have tracking enabled on components by default now, and use disable_ga4 to turn it off if needed. We do this instead of enabling tracking ourselves by passing ga4_tracking: true like we used to.

Why

Visual Changes

Changed the document list examples to reflect the new tracking

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 14, 2024 12:45 Inactive
@AshGDS AshGDS force-pushed the ga4-document-list-tracking branch from 7d82bc0 to cb033c2 Compare February 14, 2024 12:47
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 14, 2024 12:47 Inactive
@AshGDS AshGDS force-pushed the ga4-document-list-tracking branch from cb033c2 to 268ce22 Compare February 14, 2024 12:53
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 14, 2024 12:53 Inactive
@AshGDS AshGDS requested a review from JamesCGDS February 14, 2024 14:05
@AshGDS AshGDS force-pushed the ga4-document-list-tracking branch from 268ce22 to 53423ed Compare February 14, 2024 14:08
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 14, 2024 14:08 Inactive
@AshGDS AshGDS requested a review from andysellick February 16, 2024 10:14
@JamesCGDS
Copy link
Contributor

Quick question - would we want to update the With data attributes on links section in the documentation for this component? Currently it says "Note that the component does not include built in tracking. If this is required consider using the track click script"

If ga4 tracking is now enabled by default, would it be worth clarifying that the above is referring to UA?

Copy link
Contributor

@JamesCGDS JamesCGDS left a comment

Choose a reason for hiding this comment

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

Looking good (just got a small change request) 👍

spec/components/document_list_spec.rb Show resolved Hide resolved
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 19, 2024 09:30 Inactive
@AshGDS
Copy link
Contributor Author

AshGDS commented Feb 19, 2024

Thanks @JamesCGDS - should be ready for another review. I've updated that sentence in the docs as well to mention universal analytics.

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 19, 2024 12:18 Inactive
@AshGDS AshGDS force-pushed the ga4-document-list-tracking branch from f7adce0 to 5f67a98 Compare February 19, 2024 12:24
@AshGDS
Copy link
Contributor Author

AshGDS commented Feb 19, 2024

Thanks @andysellick - should be ready for review again 👍

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 19, 2024 12:25 Inactive
@andysellick
Copy link
Contributor

@AshGDS the visual diff test is mostly failing because of recently merged changes to the crown logo - if you rebase they should disappear.

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 19, 2024 16:10 Inactive
@AshGDS AshGDS force-pushed the ga4-document-list-tracking branch from 7ac5329 to c2fa750 Compare February 19, 2024 16:13
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 19, 2024 16:14 Inactive
@AshGDS
Copy link
Contributor Author

AshGDS commented Feb 19, 2024

Thanks @andysellick - should be ready to review again. The visual failures seem to be because I added two new examples to the document list docs

Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

👏

@AshGDS AshGDS force-pushed the ga4-document-list-tracking branch from c2fa750 to 32e764c Compare February 20, 2024 10:41
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 20, 2024 10:41 Inactive
@AshGDS AshGDS force-pushed the ga4-document-list-tracking branch from 32e764c to e27bc41 Compare February 20, 2024 10:48
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3874 February 20, 2024 10:49 Inactive
@JamesCGDS JamesCGDS dismissed their stale review February 20, 2024 13:58

Superseded by Andy's review

@AshGDS AshGDS merged commit 29c2315 into main Feb 20, 2024
12 checks passed
@AshGDS AshGDS deleted the ga4-document-list-tracking branch February 20, 2024 14:40
@AshGDS AshGDS mentioned this pull request Feb 27, 2024
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.

4 participants