Skip to content

Commit

Permalink
clean-sticky-ad-early-load-flag
Browse files Browse the repository at this point in the history
  • Loading branch information
zhouyx committed Jul 26, 2017
1 parent 82df5c3 commit b35ffd4
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 62 deletions.
14 changes: 2 additions & 12 deletions extensions/amp-sticky-ad/1.0/amp-sticky-ad.js
Expand Up @@ -20,16 +20,12 @@ import {Layout} from '../../../src/layout';
import {dev,user} from '../../../src/log';
import {removeElement} from '../../../src/dom';
import {toggle, computedStyle} from '../../../src/style';
import {isExperimentOn} from '../../../src/experiments';
import {
setStyle,
removeAlphaFromColor,
} from '../../../src/style';
import {whenUpgradedToCustomElement} from '../../../src/dom';

/** @const */
const EARLY_LOAD_EXPERIMENT = 'sticky-ad-early-load';

class AmpStickyAd extends AMP.BaseElement {
/** @param {!AmpElement} element */
constructor(element) {
Expand Down Expand Up @@ -140,14 +136,8 @@ class AmpStickyAd extends AMP.BaseElement {
*/
onScroll_() {
const scrollTop = this.viewport_.getScrollTop();
if (isExperimentOn(this.win, EARLY_LOAD_EXPERIMENT) && scrollTop > 1) {
this.display_();
return;
}

const viewportHeight = this.viewport_.getSize().height;
// Check user has scrolled at least one viewport from init position.
if (scrollTop > viewportHeight) {
if (scrollTop > 1) {
// Check greater than 1 because AMP set scrollTop to 1 in iOS.
this.display_();
}
}
Expand Down
45 changes: 1 addition & 44 deletions extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js
Expand Up @@ -18,7 +18,6 @@
import '../amp-sticky-ad';
import '../../../amp-ad/0.1/amp-ad';
import {poll} from '../../../../testing/iframe';
import {toggleExperiment} from '../../../../src/experiments';

describes.realWin('amp-sticky-ad 1.0 version', {
win: { /* window spec */
Expand Down Expand Up @@ -85,48 +84,7 @@ describes.realWin('amp-sticky-ad 1.0 version', {
});
});

it('should build on enough scroll dist, one more viewport ahead', () => {
const scheduleLayoutSpy = sandbox.stub(impl, 'scheduleLayoutForAd_',
() => {});
const removeOnScrollListenerSpy =
sandbox.spy(impl, 'removeOnScrollListener_');
const getScrollTopSpy = sandbox.spy();
const getSizeSpy = sandbox.spy();
const getScrollHeightSpy = sandbox.spy();

impl.viewport_.getScrollTop = function() {
getScrollTopSpy();
return 100;
};
impl.viewport_.getSize = function() {
getSizeSpy();
return {height: 50};
};
impl.viewport_.getScrollHeight = function() {
getScrollHeightSpy();
return 300;
};
impl.deferMutate = function(callback) {
callback();
};
impl.vsync_.mutate = function(callback) {
callback();
};

impl.onScroll_();
expect(getScrollTopSpy).to.have.been.called;
expect(getSizeSpy).to.have.been.called;
expect(removeOnScrollListenerSpy).to.have.been.called;
// Layout on ad is called only after fixed layer is done.
expect(scheduleLayoutSpy).to.not.have.been.called;
expect(addToFixedLayerStub).to.have.been.calledOnce;
return addToFixedLayerPromise.then(() => {
expect(scheduleLayoutSpy).to.have.been.calledOnce;
});
});

it('experiment version, should display once user scroll', () => {
toggleExperiment(win, 'sticky-ad-early-load');
it('should display once user scroll', () => {
const scheduleLayoutSpy = sandbox.stub(impl, 'scheduleLayoutForAd_',
() => {});
const removeOnScrollListenerSpy =
Expand All @@ -151,7 +109,6 @@ describes.realWin('amp-sticky-ad 1.0 version', {

impl.onScroll_();
expect(removeOnScrollListenerSpy).to.have.been.called;
toggleExperiment(win, 'sticky-ad-early-load');
// Layout on ad is called only after fixed layer is done.
expect(scheduleLayoutSpy).to.not.have.been.called;
expect(addToFixedLayerStub).to.have.been.calledOnce;
Expand Down
6 changes: 0 additions & 6 deletions tools/experiments/experiments.js
Expand Up @@ -212,12 +212,6 @@ const EXPERIMENTS = [
id: 'jank-meter',
name: 'Display jank meter',
},
{
id: 'sticky-ad-early-load',
name: 'Load sticky-ad early after user first scroll' +
'Only apply to 1.0 version',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/7479',
},
{
id: 'amp-fx-parallax',
name: 'Amp extension for a parallax effect',
Expand Down

0 comments on commit b35ffd4

Please sign in to comment.