Refactor GA4 code #3224

andysellick · 2023-11-29T09:48:02Z

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

What

Refactor some of the GA4 code in finder-frontend, based on @floehopper's previous work in #3166. This moves the logic for generating indexes for GA4 tracking on search elements (the option select and expander components) from the JS into Ruby.

Now depends upon this change in the components gem: alphagov/govuk_publishing_components#3750

Why

We want our code to be clean and readable.

Visual changes

None.

Trello card: https://trello.com/c/OdYXlYqQ/673-finder-frontend-code-optimisations

AshGDS

Looks good, though a small is change needed 👍

app/views/finders/_radio_facet.html.erb

AshGDS

LGTM 👍

andysellick · 2023-12-05T10:58:24Z

Thanks @AshGDS. I'm currently looking at expanding this PR further, which is going to involve some changes in the gem first, so I'm going to pause on this for the moment (you might want to withdrawn your approval).

AshGDS · 2023-12-05T11:38:22Z

@andysellick No worries 👍

Further changes are being added

andysellick · 2023-12-06T13:42:57Z

@AshGDS have added some more changes to this PR. This should now fully remove the JS for event listening and setting indexes on the option select and expander component buttons. It's related to this change in the gem for the option select, so will need to be tested together and I'll figure out how to deploy these two changes simultaneously.

Haven't updated the docs yet, that's another thing to look at, but should be enough to review for now.

andysellick · 2023-12-12T14:08:44Z

@AshGDS I've bumped the gem in this PR to include the needed changes to the option select component. If you have time can you give this a final once over to check I've not overlooked anything?

I haven't looked at documentation yet, I've just realised... I might do that in a separate PR for the sake of simplicity.

AshGDS

Looks good 👍

AshGDS · 2023-12-12T14:13:55Z

app/assets/javascripts/components/expander.js

-        }
+    var buttonAttributes = this.$module.getAttribute('data-button-data-attributes')
+    if (buttonAttributes) {
+      buttonAttributes = JSON.parse(buttonAttributes)


Does this need to be in a try/catch?

Good shout, will match the same thing in the option select. Have added.

- allow it to accept data attributes to be applied to the expand/collapse button - will allow us to add GA4 tracking via a Rails/component approach and remove some JS - likely to be a breaking change as will depend on some other stuff in a subsequent commit - brings the component more in line with the option select component changes coming here: alphagov/govuk_publishing_components#3750

- removes the need for some of the JS around setting indexes on JS generated elements

- includes the change to the option select component, which allows us to pass data attributes for the JS generated button

AshGDS

Thanks for adding the try/catch. I may have noticed something regarding the regex

AshGDS · 2023-12-12T14:58:59Z

app/assets/javascripts/components/expander.js

+      try {
+        buttonAttributes = JSON.parse(buttonAttributes)
+        for (var rawKey in buttonAttributes) {
+          var key = rawKey.replace(/_/i, '-').toLowerCase()


I just noticed something with this regex which also applies to the one in govuk_publishing_components

Would /i be irrelevant here as it's capturing a symbol rather than a character, so setting it to insensitive isn't necessary?

Also, should it have the /g flag instead so that it replaces every instance of an underscore in the key? With the current regex, I think it only works if the key only has one underscore?

That's really helpful, thanks @AshGDS. Yes I think you're right, although for readability I've gone with the replaceAll function. I've also updated the tests to reflect this.

Once I'm happy with this I'll make the same change on the option select component in the gem as well.

Thanks @andysellick - just to confirm, are we dropping IE11 support? If I recall correctly replaceAll doesn't work in IE11 (and it seems to not be working when I check in IE11 via BrowserStack). If we're not supporting IE11 anymore then happy to approve the change 👍

Ah thanks for spotting that, hadn't realised. Have reverted it back to replace.

andysellick · 2023-12-12T16:05:33Z

@AshGDS going to hold off on merging this until tomorrow now. Feel free to take your time if you want to look over it again - it's already proved useful!

AshGDS

Thanks @andysellick 👍 Looks like there's one small thing to fix in the test

AshGDS · 2023-12-12T16:07:58Z

spec/javascripts/components/expander-spec.js

@@ -131,7 +131,8 @@ describe('An expander module', function () {
      ga4_expandable: '',
      ga4_event: {
        event_name: 'select_content',
-        type: 'finder'
+        type: 'finder',
+        test_attribute_with_many_underscores: 'oh yes'


I think this needs to be outside of ga4_event otherwise it isn't being treated as a data-attribute, so the replace code doesn't get tested against this key.

Thanks for spotting this, have corrected. Have also updated the related PR alphagov/govuk_publishing_components#3767 with the same change.

AshGDS

LGTM 👍

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 09:48 Inactive

andysellick force-pushed the ga4-refactor branch from 04667a7 to 1202ab3 Compare November 29, 2023 12:20

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 12:20 Inactive

andysellick force-pushed the ga4-refactor branch from 1202ab3 to 521a9ea Compare November 29, 2023 12:24

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 12:24 Inactive

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 13:59 Inactive

andysellick force-pushed the ga4-refactor branch from a51c58a to aafc003 Compare November 29, 2023 13:59

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 13:59 Inactive

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 14:08 Inactive

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 14:10 Inactive

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 14:18 Inactive

andysellick force-pushed the ga4-refactor branch from f83d197 to 8ccaa26 Compare November 29, 2023 14:26

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 14:26 Inactive

andysellick force-pushed the ga4-refactor branch from 8ccaa26 to 7dd9b6f Compare November 29, 2023 16:25

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 16:25 Inactive

andysellick force-pushed the ga4-refactor branch from 7dd9b6f to 1897f74 Compare November 29, 2023 16:30

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 29, 2023 16:30 Inactive

andysellick force-pushed the ga4-refactor branch from 1897f74 to 8c3e780 Compare November 30, 2023 09:12

govuk-ci temporarily deployed to finder-frontend-pr-3224 November 30, 2023 09:12 Inactive

AshGDS requested changes Nov 30, 2023

View reviewed changes

app/views/finders/_radio_facet.html.erb Outdated Show resolved Hide resolved

andysellick force-pushed the ga4-refactor branch from 8c3e780 to 228b8aa Compare December 5, 2023 08:25

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 5, 2023 08:26 Inactive

AshGDS previously approved these changes Dec 5, 2023

View reviewed changes

andysellick mentioned this pull request Dec 5, 2023

Data attributes for option select button alphagov/govuk_publishing_components#3750

Merged

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 6, 2023 11:11 Inactive

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 6, 2023 11:13 Inactive

andysellick changed the title ~~[DO NOT MERGE] Refactor GA4 code~~ Refactor GA4 code Dec 12, 2023

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 12, 2023 14:06 Inactive

AshGDS approved these changes Dec 12, 2023

View reviewed changes

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 12, 2023 14:46 Inactive

andysellick force-pushed the ga4-refactor branch from 41ac9ec to e8adfdd Compare December 12, 2023 14:49

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 12, 2023 14:49 Inactive

andysellick added 3 commits December 12, 2023 14:55

Pass GA4 tracking attributes to components

b3a60ec

- removes the need for some of the JS around setting indexes on JS generated elements

Bump components gem

98ba043

- includes the change to the option select component, which allows us to pass data attributes for the JS generated button

andysellick force-pushed the ga4-refactor branch from e8adfdd to 98ba043 Compare December 12, 2023 14:55

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 12, 2023 14:56 Inactive

AshGDS reviewed Dec 12, 2023

View reviewed changes

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 12, 2023 15:04 Inactive

PR changes: expander JS replace all

8bf9425

andysellick force-pushed the ga4-refactor branch from 71caa96 to 8bf9425 Compare December 12, 2023 15:58

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 12, 2023 15:59 Inactive

andysellick mentioned this pull request Dec 12, 2023

Adjust option select JavaScript alphagov/govuk_publishing_components#3767

Merged

andysellick changed the title ~~Refactor GA4 code~~ [DO NOT MERGE] Refactor GA4 code Dec 12, 2023

AshGDS requested changes Dec 12, 2023

View reviewed changes

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 13, 2023 08:51 Inactive

fixup

5952026

andysellick force-pushed the ga4-refactor branch from e140e90 to 5952026 Compare December 13, 2023 08:51

govuk-ci temporarily deployed to finder-frontend-pr-3224 December 13, 2023 08:51 Inactive

andysellick changed the title ~~[DO NOT MERGE] Refactor GA4 code~~ Refactor GA4 code Dec 13, 2023

AshGDS approved these changes Dec 13, 2023

View reviewed changes

andysellick merged commit 8ccdaf6 into main Dec 13, 2023
12 checks passed

andysellick deleted the ga4-refactor branch December 13, 2023 10:17

andysellick mentioned this pull request May 21, 2024

Spike: Refactor option-select component to isolate it from GA4 functionality specific to finder-frontend #3166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GA4 code #3224

Refactor GA4 code #3224

andysellick commented Nov 29, 2023 •

edited

Loading

AshGDS left a comment

AshGDS left a comment

andysellick commented Dec 5, 2023

AshGDS commented Dec 5, 2023

andysellick commented Dec 6, 2023

andysellick commented Dec 12, 2023

AshGDS left a comment

AshGDS Dec 12, 2023

andysellick Dec 12, 2023

AshGDS left a comment

AshGDS Dec 12, 2023

andysellick Dec 12, 2023

andysellick Dec 12, 2023

AshGDS Dec 12, 2023

andysellick Dec 12, 2023

andysellick commented Dec 12, 2023

AshGDS left a comment

AshGDS Dec 12, 2023

andysellick Dec 13, 2023

AshGDS left a comment

Refactor GA4 code #3224

Refactor GA4 code #3224

Conversation

andysellick commented Nov 29, 2023 • edited Loading

What

Why

Visual changes

AshGDS left a comment

Choose a reason for hiding this comment

AshGDS left a comment

Choose a reason for hiding this comment

andysellick commented Dec 5, 2023

AshGDS commented Dec 5, 2023

andysellick commented Dec 6, 2023

andysellick commented Dec 12, 2023

AshGDS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshGDS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andysellick commented Dec 12, 2023

AshGDS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshGDS left a comment

Choose a reason for hiding this comment

andysellick commented Nov 29, 2023 •

edited

Loading