Kixer ads: Fire ad viewability when ad is in viewport #8289
Conversation
|
Hi, ampproject bot here! Here are a list of the owners that can approve your files. You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status /to lannka zhouyx
For any issues please file a bug at https://github.com/google/github-owners-bot/issues |
|
@ampprojectbot retry! |
ads/kixer.js
Outdated
| @@ -39,11 +44,34 @@ export function kixer(global, data) { | |||
| d.removeEventListener('load', kxload, false); | |||
| if (d.childNodes.length > 0) { | |||
| global.context.renderStart(); | |||
| view_interval = setInterval(function() { | |||
| kxview_check(); // Once the ad has loaded, start checking for an ad view | |||
There was a problem hiding this comment.
is such a poll necessary? observeIntersection will call the callback when there is a change.
There was a problem hiding this comment.
Hi Lanka! Thanks for your comment. It makes sense, however, this helped to make sure the ad was in the viewport for at least 1 second and prevented duplicate/early calls to the callback function.
There was a problem hiding this comment.
if all you want is "visible for 1s", then isn't setTimeout more appropriate than setInterval?
ads/kixer.js
Outdated
| const kxview_check = function(coords) { | ||
| if (coords.intersectionRect.height > coords.boundingClientRect.height / 2) { | ||
| if (viewed === false && view_timer == null) { | ||
| view_timer = setTimeout(function() { |
There was a problem hiding this comment.
Replaced setInterval for setTimeout as suggested by Lannka.
ads/kixer.js
Outdated
| d.addEventListener('load', kxload, false); | ||
| d.addEventListener('load', kxload, false); // Listen for the kixer load event | ||
|
|
||
| const kxview_check = function(coords) { |
There was a problem hiding this comment.
please use camel case for all local vars, e.g. kxviewCheck.
ads/kixer.js
Outdated
| } | ||
| }, 900); | ||
| } | ||
| in_view = true; |
There was a problem hiding this comment.
nit: you can move this out of the if block.
inView = coords.intersectionRect.height > coords.boundingClientRect.height / 2;
There was a problem hiding this comment.
Thanks Lannka. This should be out of the block.
ads/kixer.js
Outdated
| d.addEventListener('load', kxload, false); // Listen for the kixer load event | ||
|
|
||
| const kxview_check = function(coords) { | ||
| if (coords.intersectionRect.height > coords.boundingClientRect.height / 2) { |
There was a problem hiding this comment.
can you use
coords.intersectionRatio > 0.5?
ads/kixer.js
Outdated
| d.addEventListener('load', kxload, false); | ||
| d.addEventListener('load', kxload, false); // Listen for the kixer load event | ||
|
|
||
| const kxview_check = function(coords) { |
There was a problem hiding this comment.
nit: coords -> intersectionEntry
|
@ampprojectbot retry! |
|
@lannka Hi Lannka, the changes are ready. Please let me know what you think. |
ads/kixer.js
Outdated
| }, 900); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
if you want it to be visible for 900ms before firing, should you reset the timer when inView === false?
There was a problem hiding this comment.
Good point, although the timer is effectively recreated when kxviewCheck() is called again by observeIntersection.
There was a problem hiding this comment.
this is not using the memory wisely. please clear it manually
There was a problem hiding this comment.
@lannka Thanks for noticing that. The timer should now be cleared when inView is false to use memory better.
ads/kixer.js
Outdated
|
|
||
| const kxviewCheck = function(intersectionEntry) { | ||
| inView = intersectionEntry.intersectionRatio > 0.5; // Half of the unit is in the viewport | ||
| if (inView === true) { |
There was a problem hiding this comment.
you can just do if (inView)
ads/kixer.js
Outdated
| inView = intersectionEntry.intersectionRatio > 0.5; // Half of the unit is in the viewport | ||
| if (inView === true) { | ||
| if (viewed === false && viewTimer == null) { | ||
| viewTimer = setTimeout(function() { |
There was a problem hiding this comment.
add a comment what the timer is for.
ads/kixer.js
Outdated
| __kx_viewability.process_locked(data.adslot); | ||
| } | ||
| } | ||
| }, 900); |
There was a problem hiding this comment.
do you want 900ms? or 1000ms
There was a problem hiding this comment.
900ms would match our current viewability settings.
|
Hi @lannka I made the changes you suggested. Please let me know your thoughts. Thanks! |
|
@jvelis in case in missed the last comment: |
…a separate function.
|
@lannka Thanks for the last comments. Per your suggestion, I made some changes to clear the timeout more efficiently. |
We would like to improve viewability numbers by using the position in the viewport to fire the view.
Using window.context.observeIntersection API to detect the position.