-
Notifications
You must be signed in to change notification settings - Fork 8
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
fix(ga): setup default consent state #71
fix(ga): setup default consent state #71
Conversation
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.
That was fast! Thanks for doing this, just had some thoughts on making this modifiable in the future
data/google-analytics.json
Outdated
@@ -15,7 +15,7 @@ | |||
"key": "gtag" | |||
}, | |||
{ | |||
"code": "window[{{l}}]=window[{{l}}]||[];window['gtag-'+{{l}}]=function (){window[{{l}}].push(arguments);};window['gtag-'+{{l}}]('js',new Date());window['gtag-'+{{l}}]('config',{{id}})", | |||
"code": "window[{{l}}]=window[{{l}}]||[];window['gtag-'+{{l}}]=function (){window[{{l}}].push(arguments);};window['gtag-'+{{l}}]('consent', 'default', {'ad_user_data': 'denied','ad_personalization': 'denied','ad_storage': 'denied','analytics_storage': 'denied','wait_for_update': 500,});window['gtag-'+{{l}}]('js',new Date());window['gtag-'+{{l}}]('config',{{id}})", |
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 only concern with this is if anyone wants to have different consent options in the future. Instead of hardcoding, what do you think about including consent
as optionalParams instead?
Something like:
//...
"optionalParams": {
"l": "dataLayer",
"consentType": "default",
"consentValues": {},
},
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.
definitively 👍 I think nuxt-scripts shouldn't have any issues with that
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.
LGTM! Let's wait for @flashdesignory to review before we merge
"ad_storage": "denied", | ||
"analytics_storage": "denied", | ||
"wait_for_update": 500 | ||
} |
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.
Are these all the default settings? We should definitely have the default values here and then Nuxt (or any consumer) can update them as they fit
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 like denied
is the default consent state per the docs 👍
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.
🚀
@housseindjirdeh @flashdesignory I realize this was merged, so I'm late to the party, but what about #72 (comment)? Has this been considered here? In the linked docs, it says:
This makes me concerned about having |
@felixarntz Hmm that's a good point. I had thought that @huang-julien Could you submit a quick patch to make sure no values are passed in unless provided by the user? |
@housseindjirdeh @huang-julien I think we would need to be able to support conditional logic in the templating system, or not? Since right now we're just replacing |
Yeah I left a comment in the other PR about this: #72 (comment) My thinking is maybe we can pass in |
I would approach it similar to the datalayer name ( 'l' ) option. If it's null, we don't assign it. |
@flashdesignory If we can do that, that would be great. @huang-julien @felixarntz Wdyt about applying similar logic to the |
The only issue i see is that we also have params within the content. We'd probably need to format the optionnalparams too |
That won't do, the replacement system stringifies the input. This means that code injection don't work. I feel like we're reaching the limit of what we can do with JSON data. We should probably start to move away from it ? There no pros for us to keep JSON in the repo as we're already converting it to JS object with rollup in the final bundle. |
I don't think we'd need code injection. But we'd need a templating system that allows things like conditionals and loops. There are many such templating systems out there (e.g. https://mustache.github.io/), so I wonder whether any of those would make sense to use here. Though we would probably want to find something that's very lightweight, and I'm not sure what our options are in that regard. Alternatively, we come up with a simplistic custom implementation for something like Regarding removal of JSON, note that the repository also includes a PHP implementation. Using JSON is an elegant way to share the configuration in a way that's language agnostic. |
@huang-julien - just a heads up, as you noticed comments from Felix ... The great thing about having more integrations is that we have more engineers to collaborate with and I think that's a powerful side effect. |
Oh i see that make sense 😅 |
@huang-julien @flashdesignory I just gave implementing a very simple conditional template system a shot in #73. Might work just okay for our purposes. If we want to proceed with this, I can add the same logic for the PHP side in that PR, and then apply it to the |
nuxt/scripts#243
Hey 👋 this PR set the consent mode to false by default in GA.
https://developers.google.com/tag-platform/security/guides/consent?consentmode=basic#gtag.js
output with nuxt-scripts when denied by default
output with nuxt-script when we did update the consent mode