Improvements to amp-sticky-ad #5432

Closed
jasti opened this Issue Oct 5, 2016 · 10 comments

Projects

None yet

5 participants

@jasti
Collaborator
jasti commented Oct 5, 2016 edited
  • Even with DoubleClick ads that have implemented render-start, we see a translucent overlay. The overlay should not be user visible.
    sticky
  • The close button appears separate from the ad. Paint the close button the same time as the ad.
  • @ericlindley-g Can you also add your feedback about the close button and making it work similar to AMP banner? Thx.
  • Make the container always takes full width of the page, even when user set the background color to transparent.
@zhouyx zhouyx was assigned by jasti Oct 5, 2016
@jasti jasti added this to the Backlog milestone Oct 5, 2016
@jasti
Collaborator
jasti commented Oct 5, 2016

@ampproject/ads and @jpettitt FYI, incase you have additional feedback.

@zhouyx
Collaborator
zhouyx commented Oct 5, 2016

@jasti I don't quite understand. Even with render-start implemented, there will be a short period of time before we receive render-steart msg. shouldn't we put transparent overlay there?

@jasti
Collaborator
jasti commented Oct 6, 2016

As discussed offline @zhouyx, let's experiment without the transparent overlay for those networks that implement render-start. Having the transparent layout increases the perceived latency of the ad - especially on slow networks.

@jpettitt
jpettitt commented Oct 6, 2016

We've already got CSS to suppress the overlay and delay display conditional on .amp-sticky-ad-loaded. However there is still la very noticeable delay between the ad box appearing and the ad content if you scroll the page fast. In a perfect world the whole thing would be display: none until there was an actual ad to show.

amp-sticky-ad {
  z-index: 20;
  background: transparent;
  max-width: 324px;
  margin: 0 auto;
  left: calc(50% - 160px);
  opacity: 0;
}

amp-sticky-ad.amp-sticky-ad-loaded {
  background: transparent;
  opacity: 1;
}
amp-sticky-ad amp-ad {
  border: 2px solid #888;
  border-bottom: none;
  background: #fff;
}

amp-sticky-ad  .amp-sticky-ad-close-button {
    transform: scale(.7, .7) translate(20px, 23px);
    border-radius: 100%;
    border-width: 3px;
    display: none;
}

amp-sticky-ad.amp-sticky-ad-loaded .amp-sticky-ad-close-button {
  display: block;
}

@media (max-width: 320px) {
  amp-sticky-ad .amp-sticky-ad-close-button {
    transform: scale(.7, .7) translate(6px, 5px);
  }
}
@zhouyx
Collaborator
zhouyx commented Oct 6, 2016 edited

@jpettitt I think toggle display after ad load is not going to work. With display:none we wouldn't be able to schedule layout for ad. Also toggle display early is to solve the blink issue we had with ios.

@jpettitt
jpettitt commented Oct 6, 2016 edited

You mean it's not a perfect world? :-)

@zhouyx
Collaborator
zhouyx commented Oct 6, 2016

@jpettitt i guess the world is just not that perfect : )
I will be sending out a pr soon. that sticky-ad with render-start implemented ad will only be visible after ad has render started. I think it will solve the issue you mentioned for ads that support render start.

However there is still la very noticeable delay between the ad box appearing and the ad content if you scroll the page fast. In a perfect world the whole thing would be display: none until there was an actual ad to show.

@lannka
Collaborator
lannka commented Oct 6, 2016

Not yet, but soon. I mean perfect world, lol

@rudygalfi
Contributor

@lannka Can you confirm whether improvements to amp-sticky-ad are fully rolled out? I see the documentation version is now at 1.0. If so, then can we close this issue?

/cc @ericlindley-g @jasti

@zhouyx
Collaborator
zhouyx commented Dec 20, 2016

version 1.0 is fully rolled out, close issue

@zhouyx zhouyx closed this Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment