Adding margin support to placement.js for <amp-auto-ads> #7153
Conversation
|
/to @lannka |
| margins = {top: marginTop}; | ||
| } | ||
| if (marginBottom) { | ||
| (margins = margins || {}).bottom = marginBottom; |
There was a problem hiding this comment.
To avoid this, can we just default margins to { top: 0, bottom: 0 }?
There was a problem hiding this comment.
If we set the margins to anything (including 0), then it means the attemptSizeChange logic has to read the existing margins using getComputedStyle (which is expensive), to figure out what the diff is. So if we're not setting any margins it's best to just provide undefined I think.
There was a problem hiding this comment.
Or:
if (marginTop || marginBottom) {
margins = {
top: marginTop,
bottom: marginBottom
};
}
| @@ -82,8 +82,9 @@ export class Placement { | |||
| * @param {!Window} win | |||
| * @param {!Element} anchorElement | |||
| * @param {!function(!Element, !Element)} injector | |||
| * @param {!../../../src/layout-rect.LayoutMarginsChangeDef|undefined} margins | |||
There was a problem hiding this comment.
{../../../src/layout-rect.LayoutMarginsChangeDef=}
| */ | ||
| constructor(win, anchorElement, injector) { | ||
| constructor(win, anchorElement, injector, margins) { |
There was a problem hiding this comment.
we follow naming convention for opt param: opt_margins
|
@tlong2 seems Github lost Travis status pingback. It doesn't allow me to merge. The easiest way I know is to push an empty commit.. |
|
@lannka Have pushed an empty commit |
Margins are specified in the placement object within the JSON config using: