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.

Sign up for GitHub

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

Jump to bottom

Provide ad position in the page #8811

Merged
merged 1 commit into from Apr 18, 2017
Merged

Provide ad position in the page #8811

merged 1 commit into from Apr 18, 2017

Conversation

dvoytenko
Copy link
Contributor

Closes #8806.
Related to #8804.

@dvoytenko dvoytenko mentioned this pull request Apr 18, 2017
Closed
lannka
lannka requested changes Apr 18, 2017
View reviewed changes
Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

you will need to change integration.js as well.

ampcontext.js is not fully launched yet. it's behind a feature flag.

lannka
lannka reviewed Apr 18, 2017
View reviewed changes
Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

Alternatively, we can just use existing API:
window.context.initialIntersection.boundingClientRect

@dvoytenko
Copy link
Contributor Author

@lannka BCR != layoutRect: one is relative to the viewport and the other to document.

@dvoytenko
Copy link
Contributor Author

@lannka Also, could you please clarify how to change integration.js? I can't find any way to do this. I did test it on a number of 3p cases and I see my value makes it there correctly.

lannka
lannka approved these changes Apr 18, 2017
View reviewed changes
Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

took another look. integration.js is good.

@dvoytenko dvoytenko merged commit 7f976b1 into ampproject:master Apr 18, 2017
@dvoytenko dvoytenko deleted the fixes156 branch April 18, 2017 19:20
DarXector pushed a commit to DarXector/amphtml that referenced this pull request Apr 25, 2017
@DarXector
Provide ad position in the page. (ampproject#8811)
f3faca0
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
@mrjoro
Provide ad position in the page. (ampproject#8811)
ddbd9b7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithwrightbos keithwrightbos keithwrightbos approved these changes

@lannka lannka lannka approved these changes

@jridgewell jridgewell Awaiting requested review from jridgewell

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dvoytenko @keithwrightbos @lannka @ampprojectbot