Match resizing behavior of iframe inside amp-ad and amp-iframe#4840
Match resizing behavior of iframe inside amp-ad and amp-iframe#4840zhouyx merged 5 commits intoampproject:masterfrom
Conversation
| true /* success */, width, height, source, origin); | ||
| }, () => this.sendEmbedSizeResponse_( | ||
| false /* success */, width, height, source, origin)); | ||
| let newHeight, newWidth; |
There was a problem hiding this comment.
Can/should code be shared with amp-iframe?
Now that you know the code, can you add some docs about the intention (here and in amp-iframe? Will make refactoring easier.
There was a problem hiding this comment.
amp-iframechecks for attribute 'resizable' whileamp-adnot.amp-iframedoes not allow resize height to less than 100px.- 'amp-ad
sends back resize response back, whileamp-iframe` doesn't.
Are these difference intentional or are they introduced because of different PRs?
You mean add docs comments to the code? will do that
There was a problem hiding this comment.
1,2,3 are all for amp-iframe only
There was a problem hiding this comment.
We can think about if we want to eliminate those discrepancy and plan for a refactoring in a separate PR.
There was a problem hiding this comment.
For 1), amp-ad used to have resizable attribute, but is removed later. For 2) I can see the height limit definitely doesn't work for amp-ad. For 3) I think it makes sense to send response in both cases.
We need to figure out what are the desired behavior for amp-ad and amp-iframe resize respectively before we decide if the code should be shared or not. If we want to keep 1) 2) 3) different, I don't see much point of sharing code.
What's everyone's opinion on this? @cramforce @lannka @muxin
There was a problem hiding this comment.
I agree. Or we can think about code sharing in more granular level, for example, share a util function like: getContainerDimensionsForResize(containerElement, contentElement, newContentDimensions) which returns newContainerDimensions.
|
LGTM |
|
added comment to describe how we calculate newWidth/newHeight based on provided width/height. |
|
Will merge this PR first to continue with #4842. We can work on refactoring/code sharing after we have a clear plan on what to expect. |
| true /* success */, width, height, source, origin); | ||
| }, () => this.sendEmbedSizeResponse_( | ||
| false /* success */, width, height, source, origin)); | ||
| // Calculate new width and new height based on the requested size. |
There was a problem hiding this comment.
... height of the container ...
There was a problem hiding this comment.
How about this, might be shorter and clearer:
Calculate new width and height of the container to include the padding. If padding is negative, just use the requested width and height directly.
There was a problem hiding this comment.
This is clearer. Cool! I'll apply it
The iframe inside
amp-adand iframe insideamp-iframeshould behave the same during resizing. Inamp-iframewe only try to change size for parent container (amp-iframeitself), and child iframe will always fill the size of its parent.With this PR, I try to do the same thing for
amp-adand its child iframe.checked we never set
iframe_.widht/iframe_.heightelsewhere, also tested ads pages manually.This is to support
render-startAPI sending resizing info. #4842