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

Remove support and documentation for the adsense responsive feature #763

Merged
merged 1 commit into from Nov 11, 2015

Conversation

lewish
Copy link
Contributor

@lewish lewish commented Oct 27, 2015

Does not currently work as intended.
The data-ad-format="auto" behavior will attempt to set the height of the tag based on some JS logic, however the size of the ad container and iframe surrounding it aren't fixed, and won't flow vertically in the way this code expects them to.

The end result is that either excessive white-space shows below the ad or the ad element is clipped within the containing AMP iframe, unless the size of the containing element exactly matches the size that the responsive JS logic will chose.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@ampsauce
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 09c0cc0c90e141ef1cc73402c43167008afb04c5
Build details: https://travis-ci.org/ampsauce/amphtml/builds/87735326

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

@@ -58,8 +58,7 @@
type="adsense"
layout="responsive"
data-ad-client="ca-pub-8125901705757971"
data-ad-slot="7783467241"
data-ad-format="auto">
data-ad-slot="7783467241">
Copy link
Member

Choose a reason for hiding this comment

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

Could you delete this entire tag?

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 squash commits! Thanks! LGTM

@ampsauce
Copy link

ampsauce commented Nov 4, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 857824ea2d4dd1199441a2a13b1f1dcc09bd1e5f
Build details: https://travis-ci.org/ampsauce/amphtml/builds/89322409

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

@ampsauce
Copy link

ampsauce commented Nov 4, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 03683584f7486aa98178b0bd7720488925b9195f
Build details: https://travis-ci.org/ampsauce/amphtml/builds/89326301

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

@cramforce
Copy link
Member

Friendly ping :)

@ampsauce
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 35798a988d879a4c22f7ef4105d0b058e696d5b1
Build details: https://travis-ci.org/ampsauce/amphtml/builds/90576092

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

…ormat parameter as it does not work as intended.
@ampsauce
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: ef467e7f6fd58973f27406f828e7a82f05b89911
Build details: https://travis-ci.org/ampsauce/amphtml/builds/90578519

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

@ampsauce
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 6ec4cd8
Build details: https://travis-ci.org/ampsauce/amphtml/builds/90580197

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

@lewish
Copy link
Contributor Author

lewish commented Nov 11, 2015

Commits squashed.

@cramforce
Copy link
Member

Thanks! LGTM

cramforce added a commit that referenced this pull request Nov 11, 2015
Remove support and documentation for the adsense responsive feature
@cramforce cramforce merged commit b84f841 into ampproject:master Nov 11, 2015
@cramforce
Copy link
Member

Merged

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.

None yet

6 participants