Implement iframe resizing for amp-ad #1512
Conversation
|
cc @dvoytenko |
| this.element.setAttribute('height', newHeight); | ||
| this.updateHeight_(newHeight); | ||
| } | ||
| }, /* opt_is3P */true); |
There was a problem hiding this comment.
nit: space between comment and true
camelburrito
force-pushed
the
ampad
branch
2 times, most recently
from
6d65bf9 to
58960f8
Compare
|
PTAL (changes to .md file) |
| An `amp-ad` must have static layout defined as is the case with any other AMP element. However, | ||
| it's possible to resize an `amp-ad` in runtime. To do so: | ||
|
|
||
| 1. The `amp-ad` must be defined with `resizable` attribute; |
There was a problem hiding this comment.
Where do we enforce this? I think we should not use the attribute here.
There was a problem hiding this comment.
Iframes do need this as of today (so does my ads implementation),
search for isResizable_
There was a problem hiding this comment.
Probably a good idea actually. Make sure to also have the validator changes done.
There was a problem hiding this comment.
Okay - will start on the validator right after we get this in.
camelburrito
force-pushed
the
ampad
branch
4 times, most recently
from
263e627 to
c853cf6
Compare
|
PTAL |
|
LGTM |
Implement iframe resizing for amp-ad
|
Thinking about this some more: How about we do a quick follow up, where we
That way the ad can handle the expand button itself. |
|
@cramforce discussion has been started here: #1533. |
#814