amp-auto-ads: implements more versatile constraints around ad placement #8293
Conversation
|
Hi, ampproject bot here! Here are a list of the owners that can approve your files. You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status /to cramforce dvoytenko jridgewell
For any issues please file a bug at https://github.com/google/github-owners-bot/issues |
|
Re the failing diff --git a/build-system/tasks/compile.js b/build-system/tasks/compile.js
index 53e004e5..4870d566 100644
--- a/build-system/tasks/compile.js
+++ b/build-system/tasks/compile.js
@@ -141,7 +141,7 @@ function compile(entryModuleFilenames, outputDir,
// Strange access/login related files.
'build/all/v0/*.js',
// A4A has these cross extension deps.
- 'extensions/**/*-config.js',
+ 'extensions/amp-ad-network*/**/*-config.js',
'extensions/amp-ad/**/*.js',
'extensions/amp-a4a/**/*.js',
// Currently needed for crypto.js and visibility.js. |
|
Hi, ampproject bot here! Here are a list of the owners that can approve your files. You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status /to cramforce dvoytenko jridgewell
/to cramforce erwinmombay
For any issues please file a bug at https://github.com/google/github-owners-bot/issues |
|
Thanks Justin. Have incorporated your fix into this PR. |
| */ | ||
| getMinAdSpacing_() { | ||
| const adCount = this.getAdCount(); | ||
| if (this.subsequentMinSpacing_.length == 0 || |
There was a problem hiding this comment.
This can be simplified into a min search:
const adCount = this.getAdCount();
let spacing = this.initialMinSpacing_;
for (let i = 0; i < this.subsequentMinSpacing_.length; i++) {
const item = this.subsequentMinSpacing_[i];
if (item.adCount <= adCount) {
spacing = item.spacing;
}
}
return spacing;| const adTracker = new AdTracker(getExistingAds(this.win), MIN_AD_SPACING); | ||
| new AdStrategy(placements, attributes, adTracker, TARGET_AD_COUNT).run(); | ||
| const adTracker = | ||
| new AdTracker(getExistingAds(this.win), adNetwork.getAdConstraints()); |
There was a problem hiding this comment.
Why not just pass in the adNetwork?
There was a problem hiding this comment.
From a re-usability and testability perspective it's better to only pass in what you need and adTracker only needs the AdConstraints.
|
|
||
| /** | ||
| * Network specific constraints on the placement of ads on the page. | ||
| * @return {!./ad-tracker.AdConstraints} |
There was a problem hiding this comment.
Note: this line is causing gulp check-types to throw a WARNING: WARNING - Failed to load module "./ad-tracker.js"
Redefining the type here makes the problem go away, but that's not ideal. Is there a way to fix gulp check-types so it does the right thing? (or maybe I am missing something stupid?)
| const adTracker = new AdTracker(getExistingAds(this.win), MIN_AD_SPACING); | ||
| new AdStrategy(placements, attributes, adTracker, TARGET_AD_COUNT).run(); | ||
| const adTracker = | ||
| new AdTracker(getExistingAds(this.win), adNetwork.getAdConstraints()); |
There was a problem hiding this comment.
From a re-usability and testability perspective it's better to only pass in what you need and adTracker only needs the AdConstraints.
| */ | ||
| getMinAdSpacing_() { | ||
| const adCount = this.getAdCount(); | ||
| if (this.subsequentMinSpacing_.length == 0 || |
Allows networks to specify the ad density and also define a variable spacing strategy, where ads get further spaced as more are added to the page.