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

Do not hide A4A Ads in Boilerplate #5664

Merged
merged 1 commit into from Oct 18, 2016
Merged

Do not hide A4A Ads in Boilerplate #5664

merged 1 commit into from Oct 18, 2016

Conversation

tdrl
Copy link

@tdrl tdrl commented Oct 18, 2016

Fixes #5663

@jridgewell jridgewell changed the title Fix for #5663 Unhide A4A Ads Oct 18, 2016
@jridgewell jridgewell changed the title Unhide A4A Ads Do not hide A4A Ads in Boilerplate Oct 18, 2016
@lannka
Copy link
Contributor

lannka commented Oct 18, 2016

Should the ultimate fix be here?

@tdrl
Copy link
Author

tdrl commented Oct 18, 2016

Quite possibly. I've never learned how the hiding/unhiding logic really works. This was my stab at a quick fix, before we work on a real one. I agree that the real fix is to make AMP runtime unhide the ad body at an appropriate time, but I don't know enough to easily make that fix myself.

@lannka
Copy link
Contributor

lannka commented Oct 18, 2016

Sure, I can help you get started. The code I pointed should be doing the work for you (setting body visibility to visible). Can you debug through to see why it's not called (or called and ended up with unexpected behavior)?

@keithwrightbos
Copy link
Contributor

Let's get this change in for now to ensure we get a fix out ASAP. I have approved the PR.

@tdrl
Copy link
Author

tdrl commented Oct 18, 2016

Exactly. We have ads not showing in production this moment because of this bug. Let's get A fix out now and work on getting the right fix soon.

@lannka lannka added LGTM and removed NEEDS REVIEW labels Oct 18, 2016
@lannka lannka merged commit d44006d into ampproject:master Oct 18, 2016
@tdrl tdrl deleted the a4a-boilerplate-fix branch October 18, 2016 20:48
samiamorwas pushed a commit to samiamorwas/amphtml that referenced this pull request Oct 18, 2016
…mp_reddit_extension

* 'master' of https://github.com/ampproject/amphtml: (63 commits)
  Position:relative on body (ampproject#5665)
  Fix for ampproject#5663 (ampproject#5664)
  Nokta Ad Server is added as an ad type (ampproject#5550)
  RFC: Separate the load phase of AMP into multiple chunks. (ampproject#5536)
  Add OWNERS files for A4A. (ampproject#5651)
  Update DEVELOPING.md
  Call original event add/remove via interceptor (ampproject#5650)
  Fix wording on confusing steps to protect against CSRF. (ampproject#5646)
  Install runtime CSS for all amp tests (ampproject#5642)
  Implement outgoing link URL replacements. (ampproject#5628)
  Wait for window to load before installing ServiceWorker. (ampproject#5638)
  iOS wrapper viewport implementation (ampproject#5629)
  Do not use AMP Version as RTV Versions (ampproject#5474)
  Purch Ad Support for Amp-Ads (ampproject#5464)
  A11Y fix for sticky ad close button. (ampproject#5640)
  Propagate ARIA attributes to real-elements (ampproject#5590)
  Ibillboard integration (ampproject#5392)
  Add some keywords to the NPM description of the validator. (ampproject#5633)
  PWA: messaging and broadcast (ampproject#5588)
  Carousel swipe not working well on Android Firefox (ampproject#5626)
  ...
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
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

3 participants