-
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
Add GA4 tracking to some missions page blocks #4534
base: main
Are you sure you want to change the base?
Conversation
2e1fc41
to
98fe48a
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.
One question about the approach and a couple of small things.
4affa9c
to
658160e
Compare
We need to set indexes on each box even though they are defined separately in the code. We also need to preserve the columns layout, so creating a container around all the boxes and taking advatnage of the JS index setting function in our GA4 code seemed most logical
658160e
to
0b7ed1d
Compare
Fix select content typo Use each_with_index
0b7ed1d
to
ba1308b
Compare
Thanks @andysellick - should be ready to re-review. Also had to rebase as the organisation logo's render statement moved to a new template. |
d935ae7
to
4d52de5
Compare
4d52de5
to
9c36fd6
Compare
Thanks @andysellick - should be ready to review again 👍 |
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, and cleaner too. Just a couple of minor comments - also are there any tests that should be included here? Maybe one to check that if the ga4_tracking
is applied to a block that the expected attributes appear?
ga4_track_links_only: "", | ||
ga4_link: { | ||
event_name: "navigation", | ||
seciton: "Header", |
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.
Typo: seciton
@@ -382,6 +382,7 @@ Note that the `[Image: desktop.png]` syntax must be wrapped in quotes (`"[Image: | |||
Layout blocks are similar to compound blocks in that they contain nested blocks. They are used to arrange blocks on the page, and to make it easier to apply styling to the blocks. | |||
|
|||
- [Blocks Container](#blocks-container) | |||
- [Box Container](#box-container) |
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.
Presumably this documentation can be removed now?
What / Why
<div>
so the indexes could be set. Therefore there's a newbox_container
block. (I didn't want to add the tracking on to the existingcolumns_layout
<div>
that surrounds all of them already, as I don't thinkcolumns_layout
should have any tracking on it.)