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

intersectionRatio & intersectionRect substitution support #12217

Merged
merged 1 commit into from Dec 4, 2017

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Nov 27, 2017

For #12074

const intersectionRect = this.getElementIntersectionRect(opt_element);
Object.assign(state, {
'intersectionRatio': intersectionRatio,
'intersectionRect': JSON.stringify(intersectionRect),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can minimize the intersectionRect string by removing intersectionRect.left intersectionRect.top, and also by arrange numbers in order. I figure longer request string is not a big deal and we can add filter function to support this purpose. So I decide to leave as it is. Let me know if anyone has objection.

Copy link
Contributor

@lannka lannka Dec 1, 2017

Choose a reason for hiding this comment

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

I'm wondering if intersectionRect is necessary. isn't ratio enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that intersectionRect is necessary. Customer has said many times that they don't just care about "1/3 of the ad is visible" but they also want to know if it's the top 1/3, or the bottom 1/3, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

@lannka
Copy link
Contributor

lannka commented Dec 1, 2017

@zhouyx let me know if you want me to help resolve conflicts and merge

@zhouyx zhouyx merged commit 59a86aa into ampproject:master Dec 4, 2017
@zhouyx
Copy link
Contributor Author

zhouyx commented Dec 4, 2017

@lannka resolved and merged. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants