Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Amp analytics #19

wants to merge 6 commits into from

Conversation

vonloh
Copy link

@vonloh vonloh commented Sep 8, 2020

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

disable neos google analytics injections and add own tag manager logic
add GTM descriptions to APM section
Copy link
Owner

@Sebobo Sebobo left a 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') + '&gtm.url=SOURCE_URL'} data-credentials="include"></amp-analytics>
`
@if.isset = ${Configuration.setting('Shel.Blog.amp.analytics.tagManager.id')}
Copy link
Owner

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?

Copy link
Author

@vonloh vonloh Sep 10, 2020

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

Copy link
Owner

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.

Copy link
Owner

@Sebobo Sebobo left a 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') + '&gtm.url=SOURCE_URL'} data-credentials="include"></amp-analytics>
`
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.id')}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@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') + '&gtm.url=SOURCE_URL'} data-credentials="include"></amp-analytics>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<amp-analytics config={'https://www.googletagmanager.com/amp.json?id=' + Configuration.setting('Shel.Blog.analytics.tagManager.id') + '&gtm.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') + '&gtm.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')}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.id')}
@if.isset = ${Configuration.setting('Shel.Blog.analytics.tagManager.ampContainerId')}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants