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

Flexible Analytics Manager #526

Closed
wants to merge 2 commits into from
Closed

Flexible Analytics Manager #526

wants to merge 2 commits into from

Conversation

btidwell
Copy link
Contributor

@btidwell btidwell commented Apr 3, 2015

Allow analytics provider plugins to specify template tag names to allow
templates to specify where on the DOM individual providers are
rendered. (for example: google tag manager requires a data layer in
the head however, the core tag manager snippet might be best placed in
the body of the document).

Also I will follow this change with this submission of a Google Tag Manager plugin on the PencilBlue Plugins page.

Allow analytics provider plugins to specify template tag names to allow
templates to specify where on the DOM individual providers are
rendered.  (for example: google tag manager requires a data layer in
the head however, the core tag manager snippet might be best placed in
the body of the document).
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 24.21% when pulling ccedfca on btidwell:master into e54c540 on pencilblue:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 24.17% when pulling ccedfca on btidwell:master into e54c540 on pencilblue:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 24.21% when pulling ccedfca on btidwell:master into e54c540 on pencilblue:master.

@@ -112,14 +128,16 @@ module.exports = function AnalyticsManagerModule(pb) {
* instance of Localization. The last is a callback that should be called with
* two parameters. The first is an error, if occurred and the second is raw
* HTML string that represents the snippet to be executed by the analytics
* plugin.
* plugin.
* @param {String} optional name of template tag where the template service to register your snippet
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind updating the documentation to:

* @param {String} name
* @param {Function} onPageRendering 
* @param {String} [templateTag='analytics'] name of template tag where the template service to register your snippet

This will make things show up correctly when we generate our documentation using YUIDoc

@coveralls
Copy link

Coverage Status

Coverage decreased (-20.52%) to 3.69% when pulling 04e07af on btidwell:master into e54c540 on pencilblue:master.

@brianhyder
Copy link
Member

Thank you for the making the changes. I'll be looking back over this day and hopefully merging it in.

@brianhyder brianhyder self-assigned this Apr 18, 2015
@brianhyder brianhyder added this to the 0.4.1 milestone Apr 18, 2015
self.templateService.registerLocal(result.templateTag, function(flag, cb){
pb.AnalyticsManager.onPageRender(self.req, self.session, self.ls, result.providerName, cb);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

This does avoid the performance issues I mentioned but brings to light another issue with this implementation. It does not support the ability for multiple providers to be rendered under the same analytics tag. This is needed for backward compatibility.

For example, if someone were running both the google analytics plugin and the ChartBeat plugin only one would get rendered when the ^analytics^ flag is encountered in the templates. The reason is because in your loop you register a single flag name with a single provider.

The more I ponder over this use case the more I wonder if the core functionality should be to provide an analytics_header and analytics_footer flag and let the plugins themselves choose where to render with the default going to the header. Then if there are other specific use cases for inputing in the body or only in certain circumstances the plugins can register global flags and render appropriately for that specific purpose.

@brianhyder brianhyder closed this May 3, 2015
@btidwell
Copy link
Contributor Author

btidwell commented May 4, 2015

Hey Brian, Sorry for not getting back to this sooner. I think for our use case we can leave it as is for now. For Google Tag Manager, we will just leave it up to the theme template to provide the variable dataLayer and then we will let the plugin render the core GTM script via the ^analytics^ flag. Based on the way GTM works we don't want both it and Google Analytics installed side by side as GTM handles GA tracking.

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.

None yet

3 participants