-
Notifications
You must be signed in to change notification settings - Fork 541
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
Installs GTM instead of just GA4 #2801
Conversation
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
✅ Deploy Preview for cncfglossary ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
@@ -4,6 +4,12 @@ | |||
{{ partial "head.html" . }} | |||
</head> | |||
<body class="td-{{ .Kind }}"> | |||
{{ if hugo.IsProduction }} |
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.
From this line, I expected that the lines L9-L10 would not appear in Netlify preview (because it's not prod),
but in the source of the preview, those lines are rendered.
My questions are:
- Is this right? Does Hugo presume Netlify previews as prod?
(Or, it's just Hugo cannot tell Netlify previews and the real prod (https://glossary.cncf.io) apart?) - Would be there no problem if GTM tags are inserted in Netlify preview sites?
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 it's not correct. Previews should not have analytics and should not be true
for IsProduction
. It's not a huge concern, however, as it has been how this site has been working from the beginning with Google Analytics and GTM should not be any different.
I suggest we split this problem out as an independent issue and not allow it to hold up merging 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.
Here's a new issue for this problem.
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 :)
Thanks @cjyabraham !
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! Thank you!
To allow us more flexibility we want to install GTM in place of just GA4. We will also be including a HubSpot tracking code using GTM. See cncf/cncf.io#823