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

Support LCD amp-ad in multi-size #8254

Closed
wants to merge 3 commits into from
Closed

Conversation

smansour
Copy link

This check makes it only possible to do a GCD size for the primary
unit. Without the ability to put a creative wrapper on AdX we cannot
resize the container back down to 300x250.
By removing this requirement we can set the default size to 300x250 for
AdX support and allow the amp-ad container to resize up for direct sold
sponsorship larger ad sizes. The container will only resize downward
when BTF so there is no negative effect on user experience that I've
seen.
Perhaps a better way to solve this is to include a new attribute in the
amp-ad tag that allows for this option as an override. Thanks Sam
smansour@hearst

http://localhost:8000/examples/HDM-Superhero.max.html

Please pick a meaningful title for your pull request using sentence case.

Do not overuse punctuation in the title like (chore):. If it is helpful feel free to start with a project name, though, like ProjectX: Implement some feature.

Title instructions above.

Enter a succinct description of what is achieved by the PR. Ideally describe why the change is being made.

Bullet points like

  • Implements aspect X
  • Leaves out feature Y because of A
  • Improves performance by B
  • Improves accessibility by C

really help with making this more readable.

Fixes/Closes/Related-to #1 (enter issue number, except in rare cases where none exists).

This check makes it only possible to do a GCD size for the primary
unit. Without the ability to put a creative wrapper on AdX we cannot
resize the container back down to 300x250.
By removing this requirement we can set the default size to 300x250 for
AdX support and allow the amp-ad container to resize up for direct sold
sponsorship larger ad sizes. The container will only resize downward
when BTF so there is no negative effect on user experience that I've
seen.
Perhaps a better way to solve this is to include a new attribute in the
amp-ad tag that allows for this option as an override. Thanks Sam
smansour@hearst

http://localhost:8000/examples/HDM-Superhero.max.html
@ampprojectbot
Copy link
Member

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to ampproject/a4a

  • ads/google/utils.js

/to camelburrito lannka

  • examples/HDM-Superhero.html

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

@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.

@keithwrightbos
Copy link
Contributor

/cc @ampproject/a4a

@googlebot
Copy link

CLAs look good, thanks!

@smansour
Copy link
Author

removed whitespace

@glevitzky
Copy link
Contributor

I'm not sure I understand the use case here. Could you clarify under what circumstances we would want the ad slot to expand rather than contract? I'd be more comfortable if @jasti commented on this, as he was the one who formulated the size restrictions.

@jasti
Copy link
Contributor

jasti commented Mar 27, 2017

@smansour What we want to avoid, when above the fold, returning a larger size back when the AMP runtime has only reserved a smaller space.
Have you checked what happens when a 728x90 is returned but the space allocated is only 300x250?

@smansour
Copy link
Author

smansour commented Apr 4, 2017

@jasti Sorry for the late response. The main issue is that our direct sold / sponsored content comes with an ad unit that is 320x450. We can add a creative wrapper in DFP to all these units to call "window.parent.context.requestResize(%%WIDTH%%,%%HEIGHT%%);" However, the vast majority of AdX creatives are simply 300x250 units and we don't have the ability in those units to size down the 320x450 container via a creative wrapper...this puts us in an unfortunate situation where we can't run our sponsorship creative while not causing major whitespace issues. If we could have a default container of 300x250 that is able to size up to 320x450 when needed we solve the problem.

I haven't checked 728x90 since it's my understanding this is mobile only and we don't run 728x90's on mobile. For that matter we don't run them on our desktop viewport either. 970x90 is the smallest size we support.

Please advise.

@michaelkleber
Copy link

@smansour But what is your desired behavior when your 320x450 ad unit is served into a 300x250 slot that is in the viewport? AMP won't let it resize while visible, and surely you don't want it to render truncated!

The smaller-sizes-only restriction is basically the acknowledgement that whitespace may be ugly, but it's less bad than rendering a truncated creative in this situation.

@smansour
Copy link
Author

smansour commented Apr 4, 2017

@michaelkleber the ad slot where the 320x450 mobile ad loads is BTF so in my testing it always is able to resize before it enters the viewport. It would be good to have a fallback in case the user scrolled so fast that container was in view before the ad had a chance to load...not sure what the best solution for that specific scenario would be. Thoughts?

@michaelkleber
Copy link

@smansour I don't have a good fallback option to suggest, that's why we've ended up with the largest-size-first restriction. @jasti Feel free to be a brilliant PM here :-)

@jasti
Copy link
Contributor

jasti commented Apr 8, 2017

@michaelkleber's right. This is exactly the situation we didn't have an answer for in the AMP framework. Truncated ads are a no go from an advertiser's perspective.

@jasti jasti closed this Apr 8, 2017
@jasti jasti removed the in progress label Apr 8, 2017
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.

9 participants