From 298634361e02b435ffb06b5a4d97eaa37299b4a1 Mon Sep 17 00:00:00 2001 From: calebcordry Date: Thu, 22 Feb 2018 13:10:07 -0800 Subject: [PATCH 1/2] check if ad loaded --- .../amp-story/0.1/amp-story-auto-ads.js | 102 +++++++++++++----- extensions/amp-story/0.1/amp-story-page.js | 5 + 2 files changed, 82 insertions(+), 25 deletions(-) diff --git a/extensions/amp-story/0.1/amp-story-auto-ads.js b/extensions/amp-story/0.1/amp-story-auto-ads.js index 940f05a287d7..09803e6fe5ff 100644 --- a/extensions/amp-story/0.1/amp-story-auto-ads.js +++ b/extensions/amp-story/0.1/amp-story-auto-ads.js @@ -14,10 +14,15 @@ * limitations under the License. */ +import {CommonSignals} from '../../../src/common-signals'; +import {Services} from '../../../src/services'; import {StateChangeType} from './navigation-state'; +import {createElementWithAttributes} from '../../../src/dom'; import {dev, user} from '../../../src/log'; +import {dict} from '../../../src/utils/object'; -// temp before config in passed in + +// TODO(ccordry) replace these constants with user config /** @const */ const MIN_INTERVAL = 3; @@ -25,10 +30,10 @@ const MIN_INTERVAL = 3; const MAX_NUMBER = 2; /** @const */ -// const EXPERIMENT = 'amp-story-auto-ad'; +const TAG = 'amp-story-auto-ads'; /** @const */ -const TAG = 'amp-story-auto-ads'; +const AD_TAG = 'amp-ad'; export class AmpStoryAutoAds extends AMP.BaseElement { @@ -46,10 +51,13 @@ export class AmpStoryAutoAds extends AMP.BaseElement { this.interactions_ = 0; /** @private {!Array} */ - this.adElements_ = []; + this.adPageEls_ = []; /** @private {number} */ this.adsPlaced_ = 0; + + /** @private {boolean} */ + this.iscurrentAdLoaded_ = false; } /** @override */ @@ -58,6 +66,10 @@ export class AmpStoryAutoAds extends AMP.BaseElement { user().assert(ampStoryElement.tagName === 'AMP-STORY', `<${TAG}> should be child of `); + const ampdoc = this.getAmpDoc(); + Services.extensionsFor(this.win)./*OK*/installExtensionForDoc( + ampdoc, AD_TAG); + ampStoryElement.getImpl().then(impl => { this.ampStory_ = impl; this.navigationState_ = this.ampStory_.getNavigationState(); @@ -78,43 +90,84 @@ export class AmpStoryAutoAds extends AMP.BaseElement { return Promise.resolve(); } + /** * build page and start preloading * @private */ schedulePage_() { - const page = this.makeMockPage(); - this.adElements_.push(page); + const page = this.createAdPage_(); + this.adPageEls_.push(page); this.ampStory_.element.appendChild(page); - // TODO(ccordry): need to fake distance from page to force load - page.getImpl().then(impl => { this.ampStory_.addPage(impl); }); } + /** - * temporary to be replaced with real fetching + * create an `amp-story-page` containing an `amp-ad` + * @private */ - makeMockPage() { - const ampStoryAdPage = document.createElement('amp-story-page'); - const id = this.adsPlaced_ + 1; - ampStoryAdPage.id = `i-amphtml-ad-page-${id}`; - ampStoryAdPage.setAttribute('ad', ''); - ampStoryAdPage./*OK*/innerHTML = ` - -

Ad Page #${id}

-

This is ad #${id} shown in this story.

- - `; + createAdPage_() { + // TODO(ccordry) add new + const ampStoryAdPage = this.createPageElement_(); + const ampAd = this.createAdElement_(); + + ampStoryAdPage.appendChild(ampAd); + + this.iscurrentAdLoaded_ = false; + + // set up listener for ad-loaded event + ampAd.getImpl().then(impl => { + const signals = impl.signals(); + return signals.whenSignal(CommonSignals.INI_LOAD); + }).then(() => { + this.iscurrentAdLoaded_ = true; + }); + return ampStoryAdPage; } /** - * Get array containing all pages of associated story + * @return {!Element} + * @private + */ + createPageElement_() { + const id = this.adsPlaced_ + 1; + const attributes = dict({ + 'id': `i-amphtml-ad-page-${id}`, + 'ad': '', + 'distance': '1', + }); + + return createElementWithAttributes( + document, 'amp-story-page', attributes); + } + + + /** + * @return {!Element} + * @private + */ + createAdElement_() { + // TODO(ccordry) get this info (e.g. source) from config + const attributes = dict({ + 'id': 'i-amphtml-demo-ad', + 'height': '300', + 'src': '/extensions/amp-ad-network-fake-impl/0.1/data/fake_amp.json', + 'type': 'fake', + }); + + return createElementWithAttributes( + document, 'amp-ad', attributes); + } + + + /** * @private */ handleStateChange_(stateChangeEvent) { @@ -158,14 +211,13 @@ export class AmpStoryAutoAds extends AMP.BaseElement { * @private */ placeAdAfterPage_(currentPageId) { - // TODO(ccordry) make sure ad is loaded - const nextAdElement = this.adElements_[this.adElements_.length - 1]; + const nextAdPageEl = this.adPageEls_[this.adPageEls_.length - 1]; - if (!nextAdElement) { + if (!nextAdPageEl || !this.iscurrentAdLoaded_) { return; } - this.ampStory_.insertPage(currentPageId, nextAdElement.id); + this.ampStory_.insertPage(currentPageId, nextAdPageEl.id); this.adsPlaced_++; if (!this.allAdsPlaced_()) { diff --git a/extensions/amp-story/0.1/amp-story-page.js b/extensions/amp-story/0.1/amp-story-page.js index 70693367b850..2a62161b2144 100644 --- a/extensions/amp-story/0.1/amp-story-page.js +++ b/extensions/amp-story/0.1/amp-story-page.js @@ -701,6 +701,11 @@ export class AmpStoryPage extends AMP.BaseElement { }); } + + /** + * check to see if this page is a wrapper for an ad + * @return {boolean} + */ isAd() { return this.element.hasAttribute(ADVERTISEMENT_ATTR_NAME); } From 894ff43c082259372eaca59ba5dc77807605d8e4 Mon Sep 17 00:00:00 2001 From: calebcordry Date: Fri, 23 Feb 2018 09:58:34 -0800 Subject: [PATCH 2/2] add created count --- extensions/amp-story/0.1/amp-story-auto-ads.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/extensions/amp-story/0.1/amp-story-auto-ads.js b/extensions/amp-story/0.1/amp-story-auto-ads.js index 09803e6fe5ff..eb82d62e7034 100644 --- a/extensions/amp-story/0.1/amp-story-auto-ads.js +++ b/extensions/amp-story/0.1/amp-story-auto-ads.js @@ -56,8 +56,11 @@ export class AmpStoryAutoAds extends AMP.BaseElement { /** @private {number} */ this.adsPlaced_ = 0; + /** @private {number} */ + this.adPagesCreated_ = 0; + /** @private {boolean} */ - this.iscurrentAdLoaded_ = false; + this.isCurrentAdLoaded_ = false; } /** @override */ @@ -118,14 +121,14 @@ export class AmpStoryAutoAds extends AMP.BaseElement { ampStoryAdPage.appendChild(ampAd); - this.iscurrentAdLoaded_ = false; + this.isCurrentAdLoaded_ = false; // set up listener for ad-loaded event ampAd.getImpl().then(impl => { const signals = impl.signals(); return signals.whenSignal(CommonSignals.INI_LOAD); }).then(() => { - this.iscurrentAdLoaded_ = true; + this.isCurrentAdLoaded_ = true; }); return ampStoryAdPage; @@ -137,7 +140,7 @@ export class AmpStoryAutoAds extends AMP.BaseElement { * @private */ createPageElement_() { - const id = this.adsPlaced_ + 1; + const id = ++this.adPagesCreated_; const attributes = dict({ 'id': `i-amphtml-ad-page-${id}`, 'ad': '', @@ -213,7 +216,7 @@ export class AmpStoryAutoAds extends AMP.BaseElement { placeAdAfterPage_(currentPageId) { const nextAdPageEl = this.adPageEls_[this.adPageEls_.length - 1]; - if (!nextAdPageEl || !this.iscurrentAdLoaded_) { + if (!nextAdPageEl || !this.isCurrentAdLoaded_) { return; }