-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Amp analytics #19
base: master
Are you sure you want to change the base?
Amp analytics #19
Conversation
disable neos google analytics injections and add own tag manager logic
add GTM descriptions to APM section
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.
Hey, thanks for this change!
I left some feedback
<script async custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script> | ||
<amp-analytics config={'https://www.googletagmanager.com/amp.json?id=' + Configuration.setting('Shel.Blog.analytics.tagManager.id') + '>m.url=SOURCE_URL'} data-credentials="include"></amp-analytics> | ||
` | ||
@if.isset = ${Configuration.setting('Shel.Blog.amp.analytics.tagManager.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.
Wouldn't it make sense to take the setting from the Analytics package setting path?
Then it's not necessary to define it twice.
And did you use a Join
because people can add more to it?
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.
according this post, Inside google tag manager you have to create your own container type for amp pages, which then has its own container ID. Perhaps the settings name could be better, something like ampContainerID.
I've used a Join
to get access to the @position
method.
if you have a better idea i would be happy if you give me some idea how i can do it better
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.
You can always use the @position
. I think in this case I wouldn't even use AFX but just a Neos.Fusion:Tag
instead of the join and define the attributes.
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.
The configuration didn't match the setting anymore, so I added some suggested changes :)
main = afx` | ||
<amp-analytics config={'https://www.googletagmanager.com/amp.json?id=' + Configuration.setting('Shel.Blog.analytics.tagManager.id') + '>m.url=SOURCE_URL'} data-credentials="include"></amp-analytics> | ||
` | ||
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.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.
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.id')} | |
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.ampContainerId')} |
} | ||
googleTrackingCustomElement = Neos.Fusion:Join { | ||
main = afx` | ||
<amp-analytics config={'https://www.googletagmanager.com/amp.json?id=' + Configuration.setting('Shel.Blog.analytics.tagManager.id') + '>m.url=SOURCE_URL'} data-credentials="include"></amp-analytics> |
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.
<amp-analytics config={'https://www.googletagmanager.com/amp.json?id=' + Configuration.setting('Shel.Blog.analytics.tagManager.id') + '>m.url=SOURCE_URL'} data-credentials="include"></amp-analytics> | |
<amp-analytics config={'https://www.googletagmanager.com/amp.json?id=' + Configuration.setting('Shel.Blog.analytics.tagManager.ampContainerId') + '>m.url=SOURCE_URL'} data-credentials="include"></amp-analytics> |
main = afx` | ||
<script async custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script> | ||
` | ||
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.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.
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.id')} | |
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.ampContainerId')} |
In the AMP specifications it is not allowed to add custom Javascript.
If you have installed the neos/googleanalytics package aside with shel/blog, the analytics package injects a tag with js after the head tag.
This pull request remove this tag and adds the possibility to add an AMP confirmative version of a google tag manager according to this answer