Skip to content
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

Make viewability info available to amp-iframe #1231

Merged
merged 1 commit into from
Jan 21, 2016

Conversation

camelburrito
Copy link
Contributor

* @return {!LayoutRect}
*/
getInsersectionElementLayoutBox() {
return this.resources_.getLayoutBox();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this works, does it?

@camelburrito camelburrito force-pushed the viewability branch 2 times, most recently from 2b55331 to 726b38f Compare December 23, 2015 01:43
@ampsauce
Copy link

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
Build details: https://travis-ci.org/ampsauce/amphtml/builds/98440907

(Please note that this is a fully automated comment.)

/** @override */
viewportCallback(inViewport) {
// Lets the iframe know that it became visible or no longer is.
this.sendIntersection_();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make this entire method:

this.intersectionObserver_.viewportCallback(inViewport)

The logic can then go into that method and not be duplicated with amp-ad

@cramforce
Copy link
Member

Please add tests for viewability info in amp-iframe and tests for the new intersection observer class.


Done

@camelburrito camelburrito force-pushed the viewability branch 4 times, most recently from 3b0c2b7 to bcd4cfc Compare January 14, 2016 21:23
@camelburrito camelburrito changed the title [WIP]Make viewability info available to amp-iframe Make viewability info available to amp-iframe Jan 14, 2016
@camelburrito camelburrito force-pushed the viewability branch 2 times, most recently from 9e2c954 to 05c8183 Compare January 14, 2016 22:22
@camelburrito
Copy link
Contributor Author

PTAL

@cramforce
Copy link
Member

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some docs here how to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

const targetOrigin =
this.iframe_.src ? parseUrl(this.iframe_.src).origin : '*';
postMessage(
this.iframe_,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cramforce
Copy link
Member

Please add some tests for amp-iframe.


Added some

const origin = parseUrl(iframe.src).origin;
const win = iframe.ownerDocument.defaultView;
const mode = getMode();
const sentinel = opt_is3P ? 'amp-3p' : 'amp';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could unify this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@camelburrito camelburrito force-pushed the viewability branch 8 times, most recently from 9008481 to 2f17888 Compare January 21, 2016 04:24
@camelburrito
Copy link
Contributor Author

PTAL

@cramforce
Copy link
Member

WOOHOO! Taking another look!

// Triggered by context.noContentAvailable() inside the ad iframe.
listenOnce(this.iframe_, 'no-content', () => {
this.noContentHandler_();
});
}, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: here and elsewhere. Add comment like this /* opt_is3P */ true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cramforce
Copy link
Member

LGTM with one nit

camelburrito pushed a commit that referenced this pull request Jan 21, 2016
Make viewability info available to amp-iframe
@camelburrito camelburrito merged commit 9bca86e into ampproject:master Jan 21, 2016
@camelburrito camelburrito deleted the viewability branch January 21, 2016 17:54
@henel677 henel677 mentioned this pull request Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants