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

Clean sticky-ad-early-load flag #10655

Merged
merged 2 commits into from Jul 27, 2017
Merged

Clean sticky-ad-early-load flag #10655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b35ffd4
clean-sticky-ad-early-load-flag
zhouyx Jul 26, 2017
ab2a4f6
fix test
zhouyx Jul 27, 2017
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
14 changes: 2 additions & 12 deletions extensions/amp-sticky-ad/1.0/amp-sticky-ad.js
View file
Open in desktop
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
55 changes: 3 additions & 52 deletions extensions/amp-sticky-ad/1.0/test/test-amp-sticky-ad.js
View file
Open in desktop
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 @@ -54,21 +53,16 @@ describes.realWin('amp-sticky-ad 1.0 version', {
expect(impl.scrollUnlisten_).to.be.function;
});

it('should not build when scrollTop less than viewportHeight', () => {
it('should not build when scrollTop not greater than 1', () => {
const scheduleLayoutSpy = sandbox.spy(impl, 'scheduleLayout');
const removeOnScrollListenerSpy =
sandbox.spy(impl, 'removeOnScrollListener_');
const getScrollTopSpy = sandbox.spy();
const getSizeSpy = sandbox.spy();
const getScrollHeightSpy = sandbox.spy();

impl.viewport_.getScrollTop = function() {
getScrollTopSpy();
return 20;
};
impl.viewport_.getSize = function() {
getSizeSpy();
return {height: 50};
return 1;
};
impl.viewport_.getScrollHeight = function() {
getScrollHeightSpy();
Expand All @@ -79,54 +73,12 @@ describes.realWin('amp-sticky-ad 1.0 version', {
setTimeout(resolve, 0);
}).then(() => {
expect(getScrollTopSpy).to.have.been.called;
expect(getSizeSpy).to.have.been.called;
expect(scheduleLayoutSpy).to.not.have.been.called;
expect(removeOnScrollListenerSpy).to.not.have.been.called;
});
});

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 +103,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
View file
Open in desktop
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