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.

Sign up for GitHub

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

Jump to bottom

Carve out flex slot expansion for multi-size responses. #24495

Merged
merged 5 commits into from Oct 24, 2019
Merged

Carve out flex slot expansion for multi-size responses. #24495

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7a50dae
Carve out flex slot expansion if the returned creative has a size tha…
glevitzky Sep 12, 2019
a7cc159
Fix test comment.
glevitzky Oct 3, 2019
94363e8
Add comment + make check explicity for fluid as primary size.
glevitzky Oct 24, 2019
4c5fcae
Minor grammar corrections.
glevitzky Oct 24, 2019
a699343
lint fix
glevitzky Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
View file
Open in desktop
Expand Up @@ -1250,7 +1250,16 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
(returnedSizeDifferent && heightNotIncreased)
) {
this.attemptChangeSize(newHeight, newWidth).catch(() => {});
if (newWidth > width) {
if (
newWidth > width &&
// If 'fluid' were the primary requested size, ensure we do not trigger
// slot adjustment if the returned size is one of the requested multi-
// sizes. Slot adjustment should only be triggered when the creative
// size is not one of the requested sizes.
(!this.isFluidPrimaryRequest_ ||
(this.parameterSize &&
this.parameterSize.indexOf(`${newWidth}x${newHeight}`) == -1))
) {
this.adjustSlotPostExpansion_(newWidth);
}
}
Expand Down
19 changes: 17 additions & 2 deletions extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js
View file
Open in desktop
Expand Up @@ -355,8 +355,21 @@ describes.realWin('amp-ad-network-doubleclick-impl', realWinConfig, env => {
newWidth: 400,
margin: '-375px',
},
{
direction: 'ltr',
parentWidth: 300,
newWidth: 200,
margin: '',
isMultiSizeResponse: true,
},
].forEach((testCase, testNum) => {
it(`should adjust slot CSS after expanding width #${testNum}`, () => {
if (testCase.isMultiSizeResponse) {
impl.parameterSize = '320x50,200x50';
impl.isFluidPrimaryRequest_ = true;
} else {
impl.paramterSize = '200x50';
}
sandbox.stub(impl, 'attemptChangeSize').callsFake((height, width) => {
impl.element.style.width = `${width}px`;
return {
Expand Down Expand Up @@ -390,8 +403,10 @@ describes.realWin('amp-ad-network-doubleclick-impl', realWinConfig, env => {
},
});
expect(impl.element.style[`margin${dirStr}`]).to.equal(testCase.margin);
// We use a fixed '11' value for z-index.
expect(impl.element.style.zIndex).to.equal('11');
if (!testCase.isMultiSizeResponse) {
// We use a fixed '11' value for z-index.
expect(impl.element.style.zIndex).to.equal('11');
}
});
});
});
Expand Down