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
Enable adding analytics to template ad #13114
Enable adding analytics to template ad #13114
Conversation
138f65c
to
3d118c0
Compare
@@ -75,7 +78,39 @@ export class AmpAdTemplates { | |||
*/ | |||
render(templateValues, element) { | |||
return Services.templatesFor(this.win_) | |||
.findAndRenderTemplate(element, templateValues); | |||
.findAndRenderTemplate(element, templateValues) | |||
.then(renderedElement => { |
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.
is the then
clause needed?
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.
removed.
(isArray(analyticsValue) ? analyticsValue : [analyticsValue]); | ||
for (let i = 0; i < analyticsValue.length; i++) { | ||
const config = analyticsValue[i]; | ||
const analyticsEle = document.createElement('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.
should you use element.owenerDocument
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.
fixed.
} | ||
if (config['inline']) { | ||
const scriptElem = createElementWithAttributes( | ||
document, |
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.
same here
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.
fixed
@@ -151,6 +151,18 @@ export class AmpAdNetworkAdzerkImpl extends AmpA4A { | |||
|
|||
/** @override */ | |||
getAmpAdMetadata(unusedCreative) { | |||
if (this.ampCreativeJson_.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.
should you just load the analytics extension?
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 prefer not to load the extension here. AmpA4A will load extension based on metadata, I think it's better to load all extensions together.
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.
Talked offline. It's not that straightforward to load extension here. Will keep the current approach.
49b7db4
to
404d22e
Compare
* enable adding analytics to template * fix type check * rename * address comment * linter * fix test
* enable adding analytics to template * fix type check * rename * address comment * linter * fix test
Insert
<amp-analytics>
component to template ad.creativeJson should be defined like