search

xyNNN / GoogleTagManagerBundle

Google Tag Manager Bundle for Symfony 2
https://github.com/xyNNN/GoogleTagManagerBundle
GNU Lesser General Public License v3.0
27 stars 15 forks source link

Add EventListener to append Tag Manager Container automatically to HTML responses. #2

Closed tmirks closed 9 years ago

tmirks commented 9 years ago

Hey @xyNNN, I created an EventListener so you don't have to explicitly call the Twig function in your templates.

I wanted to inject Twig directly into the listener, however phpunit was complaining about a non-existent service, so I ended up injecting the DI container.

Your input / criticism on this is welcome.

xyNNN commented 9 years ago

Hey @tmirks,

thank you very much for your improvements! Before we can merge this feature i have one small comment.

  1. The Google Tag Manager Javascript Snippet is only allowed to implement it right after the HTML body tag, so I think that the decision where to append it (top or bottom) doesn't make sense.

I think it should be sufficient, if we define a boolean value if the snippet should be automatically to the HTML responses, or?

Thanks in advance!

Greets, xyNNN

tmirks commented 9 years ago

We had a bit of a discussion here about best practices and putting all javascript at the bottom of the page.

However I agree with you that Google is pretty strong on placement of the Tag Manager container, so I'll update the setting to a boolean and will probably rename it to autoAppend.

xyNNN commented 9 years ago

Hey @tmirks,

you are right that you should prefer to place all javascript snippets at the bottom of your page. But for the Google Tag Manager we have to break this rule. Thank you for the planned adjustments!

tmirks commented 9 years ago

@xyNNN, I'm working on this, lots on the go right now but I want to wrap this up soon.

xyNNN commented 9 years ago

@tmirks No Problem. It's done, when it's done! Thank you for your support! :+1:

xyNNN commented 9 years ago

I will research this issue, when you inject the twig service instead of the DI container. Currently I'm sick, so i think i will fix and merge it till Sunday. Thank you very much @tmirks :)

xyNNN commented 9 years ago

Sorry @tmirks! Since I've overcome my sickness, i will merge your pull request in the next days! :)