From e9c309c0361e1504ab1df5edf72191f09c17608e Mon Sep 17 00:00:00 2001 From: Yuxuan Zhou Date: Wed, 2 Nov 2016 16:17:34 -0700 Subject: [PATCH] make changes to sticky-ad 1.0, fix tests (#5984) * force opacity, remove amp-sticky-ad-loaded * remove padding-top --- examples/sticky.ads.0.1.amp.html | 296 ++++++++++++++++++ extensions/amp-sticky-ad/0.1/amp-sticky-ad.js | 2 +- .../0.1/test/test-amp-sticky-ad.js | 6 +- .../amp-sticky-ad/1.0/amp-sticky-ad.css | 7 +- extensions/amp-sticky-ad/1.0/amp-sticky-ad.js | 15 +- .../1.0/test/test-amp-sticky-ad.js | 13 +- testing/describes.js | 3 + 7 files changed, 310 insertions(+), 32 deletions(-) create mode 100644 examples/sticky.ads.0.1.amp.html diff --git a/examples/sticky.ads.0.1.amp.html b/examples/sticky.ads.0.1.amp.html new file mode 100644 index 000000000000..af28772d87c8 --- /dev/null +++ b/examples/sticky.ads.0.1.amp.html @@ -0,0 +1,296 @@ + + + + + Sticky Ad Test + + + + + + + + + + + +
+ +
+
+ + +
+
+ + + +
+ + +
+
+

Lorem Ipsum

+ +

+ Fusce pretium tempor justo, vitae consequat dolor maximus eget. +

+
+ +
+ + +
+

+ by +

+

PublisherName News Reporter

+

+
+
+

+ Lorem ipsum dolor sit amet, consectetur adipiscing elit. + Curabitur ullamcorper turpis vel commodo scelerisque. Phasellus + luctus nunc ut elit cursus, et imperdiet diam vehicula. + Duis et nisi sed urna blandit bibendum et sit amet erat. + Suspendisse potenti. Curabitur consequat volutpat arcu nec + elementum. Etiam a turpis ac libero varius condimentum. + Maecenas sollicitudin felis aliquam tortor vulputate, + ac posuere velit semper. +

+

+ Fusce pretium tempor justo, vitae consequat dolor maximus eget. + Aliquam iaculis tincidunt quam sed maximus. Suspendisse faucibus + ornare sodales. Nullam id dolor vitae arcu consequat ornare a + et lectus. Sed tempus eget enim eget lobortis. + Mauris sem est, accumsan sed tincidunt ut, sagittis vel arcu. + Nullam in libero nisi. +

+ +

+ Sed pharetra semper fringilla. Nulla fringilla, neque eget + varius suscipit, mi turpis congue odio, quis dignissim nisi + nulla at erat. Duis non nibh vel erat vehicula hendrerit eget + vel velit. Donec congue augue magna, nec eleifend dui porttitor + sed. Cras orci quam, dignissim nec elementum ac, bibendum et purus. + Ut elementum mi eget felis ultrices tempus. Maecenas nec sodales + ex. Phasellus ultrices, purus non egestas ullamcorper, felis + lorem ultrices nibh, in tristique mauris justo sed ante. + Nunc commodo purus feugiat metus bibendum consequat. Duis + finibus urna ut ligula auctor, sed vehicula ex aliquam. + Sed sed augue auctor, porta turpis ultrices, cursus diam. + In venenatis aliquet porta. Sed volutpat fermentum quam, + ac molestie nulla porttitor ac. Donec porta risus ut enim + pellentesque, id placerat elit ornare. +

+

+ Curabitur convallis, urna quis pulvinar feugiat, purus diam + posuere turpis, sit amet tincidunt purus justo et mi. Donec + sapien urna, aliquam ut lacinia quis, varius vitae ex. + Maecenas efficitur iaculis lorem, at imperdiet orci viverra + in. Nullam eu erat eu metus ultrices viverra a sit amet leo. + Pellentesque est felis, pulvinar mollis sollicitudin et, + suscipit eget massa. Nunc bibendum non nunc et consequat. + Quisque auctor est vel leo faucibus, non faucibus magna ultricies. + Vestibulum ante ipsum primis in faucibus orci luctus et ultrices + posuere cubilia Curae; Vestibulum tortor lacus, bibendum et + enim eu, vehicula placerat erat. Nullam gravida rhoncus accumsan. + Integer suscipit iaculis elit nec mollis. Vestibulum eget arcu + nec lectus finibus rutrum vel sed orci. +

+ +
+ + +
+ Fusce pretium tempor justo, vitae consequat dolor maximus eget. +
+
+
+ +

+ Cum sociis natoque penatibus et magnis dis parturient montes, + nascetur ridiculus mus. Nulla et viverra turpis. Fusce + viverra enim eget elit blandit, in finibus enim blandit. Integer + fermentum eleifend felis non posuere. In vulputate et metus at + aliquam. Praesent a varius est. Quisque et tincidunt nisi. + Nam porta urna at turpis lacinia, sit amet mattis eros elementum. + Etiam vel mauris mattis, dignissim tortor in, pulvinar arcu. + In molestie sem elit, tincidunt venenatis tortor aliquet sodales. + Ut elementum velit fermentum felis volutpat sodales in non libero. + Aliquam erat volutpat. +

+ +

+ Morbi at velit vitae eros congue congue venenatis non dui. + Sed lacus sem, feugiat sed elementum sed, maximus sed lacus. + Integer accumsan magna in sagittis pharetra. Class aptent taciti + sociosqu ad litora torquent per conubia nostra, per inceptos + himenaeos. Suspendisse ac nisl efficitur ligula aliquam lacinia + eu in magna. Vestibulum non felis odio. Ut consectetur venenatis + felis aliquet maximus. Class aptent taciti sociosqu ad litora + torquent per conubia nostra, per inceptos himenaeos. +

+
+
+
+
+ + + + diff --git a/extensions/amp-sticky-ad/0.1/amp-sticky-ad.js b/extensions/amp-sticky-ad/0.1/amp-sticky-ad.js index eb91cede1189..b1ead475f531 100644 --- a/extensions/amp-sticky-ad/0.1/amp-sticky-ad.js +++ b/extensions/amp-sticky-ad/0.1/amp-sticky-ad.js @@ -238,6 +238,6 @@ class AmpStickyAd extends AMP.BaseElement { } } -AMP.extension('amp-sticky-ad', AMP => { +AMP.extension('amp-sticky-ad:0.1', AMP => { AMP.registerElement('amp-sticky-ad', AmpStickyAd, CSS); }); diff --git a/extensions/amp-sticky-ad/0.1/test/test-amp-sticky-ad.js b/extensions/amp-sticky-ad/0.1/test/test-amp-sticky-ad.js index fc6a084a5f42..d80516ecaaf6 100644 --- a/extensions/amp-sticky-ad/0.1/test/test-amp-sticky-ad.js +++ b/extensions/amp-sticky-ad/0.1/test/test-amp-sticky-ad.js @@ -18,14 +18,14 @@ import {toggleExperiment} from '../../../../src/experiments'; import '../amp-sticky-ad'; import '../../../amp-ad/0.1/amp-ad'; -describes.realWin('amp-sticky-ad container 0.1', { +describes.realWin('amp-sticky-ad 0.1 version', { win: { /* window spec */ location: '...', historyOff: false, }, amp: { /* amp spec */ runtimeOn: false, - extensions: ['amp-sticky-ad'], + extensions: ['amp-sticky-ad:0.1'], }, }, env => { let win; @@ -293,7 +293,7 @@ describes.realWin('amp-sticky-ad 0.1 with real ad child', { }, amp: { /* amp spec */ runtimeOn: false, - extensions: ['amp-sticky-ad', 'amp-ad'], + extensions: ['amp-sticky-ad:0.1', 'amp-ad'], }, }, env => { let win; diff --git a/extensions/amp-sticky-ad/1.0/amp-sticky-ad.css b/extensions/amp-sticky-ad/1.0/amp-sticky-ad.css index 674864345170..75bbb9b87062 100644 --- a/extensions/amp-sticky-ad/1.0/amp-sticky-ad.css +++ b/extensions/amp-sticky-ad/1.0/amp-sticky-ad.css @@ -23,6 +23,9 @@ amp-sticky-ad { z-index: 11; max-height: 100px !important; box-sizing: border-box; + opacity: 1 !important; + background-image: none !important; + background-color: #fff; } .-amp-sticky-ad-layout { @@ -39,10 +42,6 @@ amp-sticky-ad[visible] { visibility: visible !important; } -.amp-sticky-ad-loaded { - background-color: #fff; -} - .-amp-sticky-ad-layout > amp-ad { display: block; } diff --git a/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js b/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js index c96e5260371f..d1cb9e2b831a 100644 --- a/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js +++ b/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js @@ -24,10 +24,6 @@ import { setStyle, removeAlphaFromColor, } from '../../../src/style'; -import {isExperimentOn} from '../../../src/experiments'; - -/** @private @const {string} */ -const UX_EXPERIMENT = 'amp-sticky-ad-better-ux'; class AmpStickyAd extends AMP.BaseElement { /** @param {!AmpElement} element */ @@ -89,7 +85,6 @@ class AmpStickyAd extends AMP.BaseElement { /** @override */ unlayoutCallback() { this.viewport_.updatePaddingBottom(0); - this.element.classList.remove('amp-sticky-ad-loaded'); return true; } @@ -175,7 +170,6 @@ class AmpStickyAd extends AMP.BaseElement { this.vsync_.mutate(() => { // Set sticky-ad to visible and change container style this.element.setAttribute('visible', ''); - this.element.classList.add('amp-sticky-ad-loaded'); this.forceOpacity_(); }); }); @@ -216,13 +210,6 @@ class AmpStickyAd extends AMP.BaseElement { * @private */ forceOpacity_() { - if (!isExperimentOn(this.win, UX_EXPERIMENT)) { - return; - } - // TODO(@zhouyx): Move the opacity style to CSS after remove experiments - // Note: Use setStyle because we will remove this line later. - setStyle(this.element, 'opacity', '1 !important'); - setStyle(this.element, 'background-image', 'none'); const backgroundColor = this.win./*OK*/getComputedStyle(this.element) .getPropertyValue('background-color'); const newBackgroundColor = removeAlphaFromColor(backgroundColor); @@ -235,6 +222,6 @@ class AmpStickyAd extends AMP.BaseElement { } } -AMP.extension('amp-sticky-ad', AMP => { +AMP.extension('amp-sticky-ad:1.0', AMP => { AMP.registerElement('amp-sticky-ad', AmpStickyAd, CSS); }); diff --git a/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js b/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js index 763a975a3c2a..f12e374b3bb3 100644 --- a/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js +++ b/extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js @@ -14,18 +14,17 @@ * limitations under the License. */ -import {toggleExperiment} from '../../../../src/experiments'; import '../amp-sticky-ad'; import '../../../amp-ad/0.1/amp-ad'; -describes.realWin('amp-sticky-ad container 1.0', { +describes.realWin('amp-sticky-ad 1.0 version', { win: { /* window spec */ location: '...', historyOff: false, }, amp: { /* amp spec */ runtimeOn: false, - extensions: ['amp-sticky-ad'], + extensions: ['amp-sticky-ad:1.0'], }, }, env => { let win; @@ -206,12 +205,9 @@ describes.realWin('amp-sticky-ad container 1.0', { expect(layoutAdSpy).to.be.called; impl.ad_.dispatchEvent(new Event('amp:load:end')); expect(ampStickyAd).to.have.attribute('visible'); - expect(ampStickyAd.classList.contains('amp-sticky-ad-loaded')) - .to.be.true; }); it('should not allow container to be set semi-transparent', () => { - toggleExperiment(window, 'amp-sticky-ad-better-ux', true); ampStickyAd.setAttribute('style', 'background-color: rgba(55, 55, 55, 0.55) !important'); impl.vsync_.mutate = function(callback) { @@ -221,11 +217,9 @@ describes.realWin('amp-sticky-ad container 1.0', { impl.ad_.dispatchEvent(new Event('amp:load:end')); expect(window.getComputedStyle(ampStickyAd) .getPropertyValue('background-color')).to.equal('rgb(55, 55, 55)'); - toggleExperiment(window, 'amp-sticky-ad-better-ux', false); }); it('should not allow container to be set to transparent', () => { - toggleExperiment(window, 'amp-sticky-ad-better-ux', true); ampStickyAd.setAttribute('style', 'background-color: transparent !important'); impl.vsync_.mutate = function(callback) { @@ -235,7 +229,6 @@ describes.realWin('amp-sticky-ad container 1.0', { impl.ad_.dispatchEvent(new Event('amp:load:end')); expect(window.getComputedStyle(ampStickyAd) .getPropertyValue('background-color')).to.equal('rgb(0, 0, 0)'); - toggleExperiment(window, 'amp-sticky-ad-better-ux', false); }); }); @@ -293,7 +286,7 @@ describes.realWin('amp-sticky-ad 1.0 with real ad child', { }, amp: { /* amp spec */ runtimeOn: false, - extensions: ['amp-sticky-ad', 'amp-ad'], + extensions: ['amp-sticky-ad:1.0', 'amp-ad'], }, }, env => { let win; diff --git a/testing/describes.js b/testing/describes.js index 1a4e136db69d..557cfab3e084 100644 --- a/testing/describes.js +++ b/testing/describes.js @@ -451,6 +451,9 @@ class AmpFixture { } if (this.spec.amp.extensions) { this.spec.amp.extensions.forEach(extensionId => { + if (extensionId.indexOf(':') != -1) { + extensionId = extensionId.substring(0, extensionId.indexOf(':')); + } resetScheduledElementForTesting(win, extensionId); }); }