Add SSR support for fluid layout #1232
Conversation
| @@ -79,6 +80,13 @@ function apply(layout, width, height, node) { | |||
| case 'container': | |||
| // Do nothing here. | |||
| break; | |||
| case 'fluid': | |||
| if (width.isSet) { | |||
| styles = `width:${width.numeral}${width.unit};`; | |||
There was a problem hiding this comment.
When looking into it, it seems width isn't actually used or respected by fluid ads. Instead, there's a width: 100% !important set by https://github.com/ampproject/amphtml/blob/0320666d03db37c4e0e363b55f828aa807ae321c/extensions/amp-ad/0.1/amp-ad.css#L76-L78
There was a problem hiding this comment.
The Fluid docs say:
It will always occupy the maximum available width, and its height will be determined relative to that width.
But also:
Note also that the width attribute is optional, and can be specified. When specified, the fluid creative will always occupy that width (unless used in conjunction with multi-size).
So it seem there is still a case where the width would be used?
There was a problem hiding this comment.
I don't believe it can be, due to the !important override?
There was a problem hiding this comment.
Since the selector is for amp-ad[data-a4a-upgrade-type='amp-ad-network-doubleclick-impl'][height='fluid'], is the data-a4a-upgrade-type="amp-ad-network-doubleclick-impl" attribute always added at runtime?
There was a problem hiding this comment.
Yup, doubleclick is the only thing that supports fluid, and that attr gets injected when amp-ad loads.
| @@ -79,6 +80,10 @@ function apply(layout, width, height, node) { | |||
| case 'container': | |||
| // Do nothing here. | |||
| break; | |||
| case 'fluid': | |||
| styles += 'height:0;'; | |||
There was a problem hiding this comment.
Nit: we had width:100%;height:0px; in https://github.com/ampproject/amphtml/pull/34560/files#diff-ec6474c38021deb5cc02df92e38f7ae44a0bbda93384581872671a9c1c882d6fR292
There was a problem hiding this comment.
Added in d9876e2, but 0 seems better than 0px
westonruter
force-pushed
the
add/fluid-layout-ssr-support
branch
from
8581b15
to
200a100
Compare
Co-authored-by: Alain Schlesser <[email protected]>
* Add SSR support for fluid layout * Remove unnecessary `width` when applying `fluid` layout * Add `width` to applied fluid layout * Remove needless unit from zero fluid height * Add transforms_layout_fluid test * Remove needless string concatenation Co-authored-by: Alain Schlesser <[email protected]> Co-authored-by: Alain Schlesser <[email protected]>
The use of the
fluidlayout currently blocks server-side rendering since it is not among theSUPPORTED_LAYOUTS:amp-toolbox/packages/optimizer/lib/transformers/ApplyLayout.js
Lines 30 to 40 in 3ec7bd4
The
fluidlayout is not ideal because it inherently involves a layout shift since its dimensions get determined at runtime, and it will not get rendered when it is initially in the viewport (per docs). Nevertheless, when this layout is required it would be preferable for it to not prevent other parts of the page from benefiting from SSR.Implementation provided by @jridgewell.
Merging this PR is blocked by the AMP validator being updated, specifically to modify
validateSsrLayout()as follows:const validInternalClasses = Object.create(null); validInternalClasses[getLayoutClass(layout)] = 0; if (isLayoutSizeDefined(layout)) { // i-amphtml-layout-size-defined validInternalClasses[getLayoutSizeDefinedClass()] = 0; } + if ('fluid' === layout) { + validInternalClasses['i-amphtml-layout-awaiting-size'] = 0; + }cf. b/184254585