Kixer ads: Fire ad viewability when ad is in viewport by jvelis-nexstar · Pull Request #8289 · ampproject/amphtml

jvelis-nexstar · 2017-03-21T17:35:56Z

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.

ampprojectbot · 2017-03-21T17:35:58Z

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

ads/kixer.js

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

jvelis-nexstar · 2017-03-28T00:15:51Z

@ampprojectbot retry!

lannka · 2017-03-28T00:19:19Z

ads/kixer.js

    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


is such a poll necessary? observeIntersection will call the callback when there is a change.

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.

if all you want is "visible for 1s", then isn't setTimeout more appropriate than setInterval?

jvelis-nexstar · 2017-04-03T14:35:39Z

ads/kixer.js

+  const kxview_check = function(coords) {
+    if (coords.intersectionRect.height > coords.boundingClientRect.height / 2) {
+      if (viewed === false && view_timer == null) {
+        view_timer = setTimeout(function() {


Replaced setInterval for setTimeout as suggested by Lannka.

lannka · 2017-04-03T20:59:11Z

ads/kixer.js

-  d.addEventListener('load', kxload, false);
+  d.addEventListener('load', kxload, false); // Listen for the kixer load event
+
+  const kxview_check = function(coords) {


please use camel case for all local vars, e.g. kxviewCheck.

lannka · 2017-04-03T21:00:04Z

ads/kixer.js

+          }
+        }, 900);
+      }
+      in_view = true;


nit: you can move this out of the if block.

inView = coords.intersectionRect.height > coords.boundingClientRect.height / 2;

Thanks Lannka. This should be out of the block.

lannka · 2017-04-03T21:00:35Z

ads/kixer.js

+  d.addEventListener('load', kxload, false); // Listen for the kixer load event
+
+  const kxview_check = function(coords) {
+    if (coords.intersectionRect.height > coords.boundingClientRect.height / 2) {


can you use
coords.intersectionRatio > 0.5?

lannka · 2017-04-03T21:01:13Z

ads/kixer.js

-  d.addEventListener('load', kxload, false);
+  d.addEventListener('load', kxload, false); // Listen for the kixer load event
+
+  const kxview_check = function(coords) {


nit: coords -> intersectionEntry

jvelis-nexstar · 2017-04-11T15:26:47Z

@ampprojectbot retry!

jvelis-nexstar · 2017-04-12T18:18:07Z

@lannka Hi Lannka, the changes are ready. Please let me know what you think.

jvelis-nexstar · 2017-04-13T18:31:34Z

Hi @zhouyx and @lannka , my changes are ready. I would appreciate if someone could take a look at my PR whenever possible. Thanks for your help guys!

lannka · 2017-04-17T23:03:41Z

ads/kixer.js

+        }, 900);
+      }
+    }
+  };


if you want it to be visible for 900ms before firing, should you reset the timer when inView === false?

Good point, although the timer is effectively recreated when kxviewCheck() is called again by observeIntersection.

this is not using the memory wisely. please clear it manually

@lannka Thanks for noticing that. The timer should now be cleared when inView is false to use memory better.

lannka · 2017-04-17T23:04:06Z

ads/kixer.js

+
+  const kxviewCheck = function(intersectionEntry) {
+    inView = intersectionEntry.intersectionRatio > 0.5; // Half of the unit is in the viewport
+    if (inView === true) {


you can just do if (inView)

lannka · 2017-04-17T23:04:26Z

ads/kixer.js

+    inView = intersectionEntry.intersectionRatio > 0.5; // Half of the unit is in the viewport
+    if (inView === true) {
+      if (viewed === false && viewTimer == null) {
+        viewTimer = setTimeout(function() {


add a comment what the timer is for.

lannka · 2017-04-17T23:04:38Z

ads/kixer.js

+              __kx_viewability.process_locked(data.adslot);
+            }
+          }
+        }, 900);


do you want 900ms? or 1000ms

900ms would match our current viewability settings.

jvelis-nexstar · 2017-04-25T14:38:39Z

Hi @lannka I made the changes you suggested. Please let me know your thoughts. Thanks!

lannka · 2017-05-04T22:16:44Z

@jvelis in case in missed the last comment:
#8289 (comment)

…a separate function.

jvelis-nexstar · 2017-05-09T21:53:06Z

@lannka Thanks for the last comments. Per your suggestion, I made some changes to clear the timeout more efficiently.

ampwafflebot added the in progress label Mar 21, 2017

jvelis-nexstar mentioned this pull request Mar 21, 2017

Kixer ads: Fire viewability when ad becomes visible #8126

Closed

lannka self-assigned this Mar 28, 2017

lannka suggested changes Mar 28, 2017

View reviewed changes

jvelis-nexstar commented Apr 3, 2017

View reviewed changes

lannka suggested changes Apr 3, 2017

View reviewed changes

lannka reviewed Apr 17, 2017

View reviewed changes

jvelis-nexstar added 6 commits May 9, 2017 15:34

Changed setInterval for setTimeout

b9b3799

Changed var names to camel case and code clean up

c37aab9

Added comments

3c7f056

Fixed condition and added comment on timer

f9c4225

Added comment to run test again

25ae22a

Clear the timeout when the ad is not in view. Moved the view call to …

58a2e1a

…a separate function.

googlebot added the cla: yes label May 9, 2017

Add comment to restart tests

4532a6c

lannka approved these changes May 9, 2017

View reviewed changes

lannka merged commit 62e4531 into ampproject:master May 9, 2017

ampprojectbot added the PR use: In Beta / Experimental label May 11, 2017

ampprojectbot added the PR use: In Stable label May 23, 2017

Conversation

jvelis-nexstar commented Mar 21, 2017

Uh oh!

ampprojectbot commented Mar 21, 2017

Uh oh!

jvelis-nexstar commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvelis-nexstar commented Apr 11, 2017

Uh oh!

jvelis-nexstar commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvelis-nexstar commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvelis-nexstar commented Apr 25, 2017

Uh oh!

lannka commented May 4, 2017

Uh oh!

jvelis-nexstar commented May 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jvelis-nexstar commented Apr 12, 2017 •

edited

Loading