-
Notifications
You must be signed in to change notification settings - Fork 20
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
Simplify the way ga4 tracking is added to accordions #3082
Conversation
638228b
to
6a1428e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My brain's gone a bit foggy this afternoon so this might not be particularly thorough or coherent, but looks good, like the approach, this will definitely save us some effort - just a few comments.
app/views/govuk_publishing_components/components/docs/accordion.yml
Outdated
Show resolved
Hide resolved
app/views/govuk_publishing_components/components/_accordion.html.erb
Outdated
Show resolved
Hide resolved
4bfdc02
to
2b1c5c6
Compare
2b1c5c6
to
0aef91d
Compare
Thanks @andysellick @JamesCGDS - I think I've resolved the changes you requested, feel free to re-review when you have a chance. Will squash the commits once everything is approved 👍 |
var showAllAttributesGa4 = this.$module.getAttribute('data-ga4-show-all-attributes') | ||
if (showAllAttributesGa4) { | ||
showAll = this.$module.querySelector(this.showAllControls) | ||
showAll.setAttribute('data-ga4-event', showAllAttributesGa4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing these attributes to the component and then moving them onto the showall button, couldn't we just hardcode them? Something like this:
// look for data attributes to put onto the 'show/hide all' link
var showAll = this.$module.querySelector(this.showAllControls)
var showAllAttributes = this.$module.getAttribute('data-show-all-attributes')
if (showAllAttributes) {
try {
var values = JSON.parse(showAllAttributes)
var keys = Object.keys(values)
for (var i = 0; i < keys.length; i++) {
showAll.setAttribute('data-' + keys[i], values[keys[i]])
}
} catch (e) {
console.error('Could not read accordion data attributes error: ' + e.message, window.location)
}
}
// if GA4 is enabled, set attributes on 'show all sections' for tracking using ga4-event-tracker
var isGa4Enabled = this.$module.hasAttribute('data-ga4-expandable')
if (isGa4Enabled) {
var indexTotal = this.$module.querySelectorAll('.govuk-accordion__section').length
var showAllAttributesGa4 = {"event_name":"select_content","type":"accordion","index":0,"index_total": indexTotal}
showAll.setAttribute('data-ga4-event', JSON.stringify(showAllAttributesGa4))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andysellick - I've added this change and then will move it to ga4-core.js
once the link tracker PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still need moving to ga4-core.js
? I can't think what else will need this right now. Maybe move it later when we need it, shouldn't have to block this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is due to ga4-core
not being initialised in govuk_publishing_components
, so the accordion would crash on the developer frontend which showcases the component. Therefore we decided to share the function at a later date.
fda8e85
to
bc78bd9
Compare
bc78bd9
to
4871e59
Compare
4871e59
to
b38e41e
Compare
b38e41e
to
7e88c8a
Compare
app/assets/javascripts/govuk_publishing_components/components/accordion.js
Show resolved
Hide resolved
There was a problem hiding this 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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Just two minor documentation comments then I think we're good to go 👍
@@ -181,21 +181,33 @@ examples: | |||
<a class="govuk-link" href="#">Retiring your service</a> | |||
</li> | |||
</ul>' | |||
with_ga4_tracking: | |||
description: | | |||
Allows you to add GA4 tracking to an accordion. This will add a data module and data-attributes with JSONs to the accordion. See the `ga4-event-tracker` docs for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a link to these docs here?
with_data_attributes: | ||
description: | | ||
Adds custom data attributes to each section of the accordion. Accepts a hash, so multiple attributes can be added. | ||
|
||
The `data_attributes` option applies attributes to the outermost element in the accordion. Each item can also have a `data_attributes` hash, which are placed on the `button` that triggers the opening and closing - useful for differentiating between each section of the accordion. | ||
|
||
Data attributes can be added to the 'Show/hide all' link using the `data_attributes_show_all` option, primarily where custom tracking is required. These attributes are read from the accordion markup and then added to the link by JavaScript (which is how the link is created). More details on how this can be used with the GA4 event tracking can be found in the 'Advanced' section of the [event tracking documentation](https://github.com/alphagov/govuk_publishing_components/blob/main/docs/analytics-ga4/ga4-event-tracker.md). | ||
Data attributes can be added to the 'Show/hide all' link using the `data_attributes_show_all` option, primarily where custom tracking is required. These attributes are read from the accordion markup and then added to the link by JavaScript (which is how the link is created). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this now only applies (or is needed) for UA instead of GA4? Is it worth clarifying this here?
Previously we were passing a GTM object to each accordions render statement inside of a frontend app. Instead, it's simpler to just pass a boolean in the frontend apps, and then we handle the GTM object inside of govuk_publishing_components. This gives us the benefit of only having to maintain our GTM code in one repo. The old show_all_attributes took a JSON which contained JSONS, and appended each one as a data-attribute to the 'Show all link'. This broke the refactored accordion tracking, as we now only pass through one JSON, not a JSON which contains JSONs. The old show_all_attributes is no longer needed for GA4, but we can't modify the code for it as it is now being used for UA. Therefore I've separated the new ga4 functionality out, which as a side effect gives us a more accurate data attribute name, 'data-ga4-show-all-attributes'.
ff42464
to
c90f379
Compare
Hi @andysellick / @JamesCGDS
Would you be able to review this? Thanks 👍
What
Previously we were passing a GTM object to each accordion's render statement inside of a frontend app's template. See an example here:
https://github.com/alphagov/collections/blob/2304bf53592f39e35270413fdb27c00c7cb3190a/app/views/world_wide_taxons/accordion.html.erb#L31
Instead, it's simpler to just pass a boolean -
ga4_tracking: true
- in the accordion render of these frontend apps, and then we handle the GTM object in one place inside ofgovuk_publishing_components
instead.We were also using the accordion's class to check wether we tracked
aria-expanded
. We are now using a data attribute for this, calleddata-ga4-expandable
This isn't a breaking change as the old code is still in the repo, as that code was used to add some new UA tracking so we have to leave it in.
Why
data-ga4-
attributes which is better.Visual Changes
None.