feat(gtm): add optionnal consent mode #72
Conversation
huang-julien
changed the title
There was a problem hiding this comment.
LGTM. Will defer to @flashdesignory for a sanity check before we merge
|
Setting this to draft mode until #73 lands on main branch |
|
@huang-julien Just merged #73, so you should now be able to use the |
|
Woohoo 🎉🎉 thank you ! I'll update the PR tomorrow ! |
There was a problem hiding this comment.
@huang-julien Looks good except two things, I think specifically the inclusion of consentValues in the schema optionalParams is important.
Once this is merged, the PHPUnit tests will probably fail, I can follow up with that. I just realized the reason that this isn't flagged here is because we have an allowlist of file types in there, and the data/*.json files aren't among them - so we should fix so that in the future we'll find out "automatically" prior to merging.
data/google-tag-manager.json
Outdated
| "optionalParams": { | ||
| "l": "dataLayer" | ||
| "l": "dataLayer", | ||
| "consentType": "default" |
There was a problem hiding this comment.
I think consentValues needs to be present here, like with Analytics? https://github.com/GoogleChromeLabs/third-party-capital/blob/main/data/google-analytics.json#L23
| "consentType": "default" | |
| "consentType": "default", | |
| "consentValues": null |
There was a problem hiding this comment.
oh, consentValues can be undefined. null and undefined both count as falsy boolean expression. We can also remove it from analytics so the JSON can be slightly lighter.
There was a problem hiding this comment.
oh, consentValues can be undefined. null and undefined both count as falsy boolean expression. We can also remove it from analytics so the JSON can be slightly lighter.
Generally makes sense, however wouldn't it be more intuitive to still have those be explicitly listed in optionalParams? I think it helps DX to be able to quickly scan the params and optionalParams and know what's available. Otherwise you'll have to look at the less obvious and harder to parse "code" value.
I don't think it technically makes a difference whether they're explicitly defined or not, but personally I think there's a benefit of keeping them in there.
There was a problem hiding this comment.
I get it now, llet's keep it 👍
huang-julien
changed the title
linked to nuxt/scripts#243
same as #71 but for GTM
Seems like this require to push
argumentsinto the datalayer array. 🤷♂️Result with the GTM debugger: