Make viewability info available to amp-iframe #1231
Conversation
| * @return {!LayoutRect} | ||
| */ | ||
| getInsersectionElementLayoutBox() { | ||
| return this.resources_.getLayoutBox(); |
There was a problem hiding this comment.
I don't believe this works, does it?
camelburrito
force-pushed
the
viewability
branch
2 times, most recently
from
2b55331
to
726b38f
Compare
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 726b38f26a8915060d75aa8a08cdd0fb872976b1 (Please note that this is a fully automated comment.) |
camelburrito
force-pushed
the
viewability
branch
from
726b38f
to
12be75b
Compare
camelburrito
force-pushed
the
viewability
branch
4 times, most recently
from
31ed2c1
to
7fcfa63
Compare
camelburrito
force-pushed
the
viewability
branch
from
7fcfa63
to
ffbbdb9
Compare
| /** @override */ | ||
| viewportCallback(inViewport) { | ||
| // Lets the iframe know that it became visible or no longer is. | ||
| this.sendIntersection_(); |
There was a problem hiding this comment.
Just make this entire method:
this.intersectionObserver_.viewportCallback(inViewport)
The logic can then go into that method and not be duplicated with amp-ad
|
Please add tests for viewability info in Done |
camelburrito
force-pushed
the
viewability
branch
4 times, most recently
from
3b0c2b7
to
bcd4cfc
Compare
camelburrito
changed the title
camelburrito
force-pushed
the
viewability
branch
2 times, most recently
from
9e2c954
to
05c8183
Compare
|
PTAL |
|
This looks very good. Are you in tomorrow? Would like to do some manual testing with you, but I think otherwise this is about ready to be merged. |
| @@ -61,3 +63,109 @@ export function getIntersectionChangeEntry( | |||
| intersectionRect, | |||
| }; | |||
| } | |||
|
|
|||
|
|
|||
| export class IntersectionObserver extends Observable { | |||
There was a problem hiding this comment.
Please add some docs here how to use it.
| const targetOrigin = | ||
| this.iframe_.src ? parseUrl(this.iframe_.src).origin : '*'; | ||
| postMessage( | ||
| this.iframe_, |
|
Please add some tests for Added some |
| const origin = parseUrl(iframe.src).origin; | ||
| const win = iframe.ownerDocument.defaultView; | ||
| const mode = getMode(); | ||
| const sentinel = opt_is3P ? 'amp-3p' : 'amp'; |
camelburrito
force-pushed
the
viewability
branch
8 times, most recently
from
9008481
to
2f17888
Compare
|
PTAL |
|
WOOHOO! Taking another look! |
| // Triggered by context.noContentAvailable() inside the ad iframe. | ||
| listenOnce(this.iframe_, 'no-content', () => { | ||
| this.noContentHandler_(); | ||
| }); | ||
| }, true); |
There was a problem hiding this comment.
Nit: here and elsewhere. Add comment like this /* opt_is3P */ true
|
LGTM with one nit |
camelburrito
force-pushed
the
viewability
branch
from
2f17888
to
f119bc1
Compare
Make viewability info available to amp-iframe
#1158