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
mediaimpact: renderStart API #5333
Conversation
/to @lannka |
@@ -25,6 +25,15 @@ import {loadScript} from '../3p/3p'; | |||
|
|||
export function mediaimpact(global, data) { | |||
global.fif = false; | |||
/* eslint google-camelcase/google-camelcase: 0 */ | |||
global.sas_loadHandler = function(f) { | |||
if (!f.hasAd) { |
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.
nitpick: can you reverse the if-else
(avoid negative check)?
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.
maybe, for what benefit? Haven't heard about avoid negative check till now and cannot find something about it on web...
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.
oh, it's just more intuitive to read :-). No worry, I'm not strong about this.
if (!f.hasAd) { | ||
global.context.noContentAvailable(); | ||
} else { | ||
//global.context.requestResize(f.crea1.width, f.crea1.height); |
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.
please remove
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.
doesn't minify remove this automatically for prod? I put this line there because it isn't working. I tried locally without success using ads example page so I put it there for review.
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 use:
global.context.renderStart({width: f.crea1.width, height: f.crea1.height});
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.
OK, will do, but doesn't work either on testpage
http://localhost:8000/examples/ads.amp.max.html#mediaimpact
@@ -39,7 +48,7 @@ export function mediaimpact(global, data) { | |||
view: 'm', | |||
async: true, | |||
}; | |||
loadScript(global, 'https://ec-ns.sascdn.com/diff/251/divscripte/amp.js?dom=' + window.context.location.host, () => { | |||
loadScript(global, 'https://ec-ns.sascdn.com/diff/251/pages/amp_default.js', () => { |
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.
do you want to leverage preload? see _config.js
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.
THX, I thought we added the root here instead. Then I have to change this in _config.js either. I want to reduced requests to speed up the integration a little bit...
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.
Not sure I fully understand you. Prefetch does help you speed up a little bit. Of course it's totally optional, if you want to reduce the network pings.
08bc35a
to
f889bda
Compare
added loadHandler functionality to support renderStart API
changed lib file url