FIX: Fully re-render ads when navigating between pages (#188)

In the past, the ad plugin relied on two side-effects to achieve this behaviour:

1. Components being fully destroyed/rendered when navigating between pages. This stopped working when Discourse core moved to the more efficient 'loading slider' UI

2. The `listLoading` argument. This was an implementation detail of the old discovery routing infrastructure. Core recently overhauled this and removed the `listLoading` argument, because loading is now handled properly by the Ember router.

Instead of these two properties, we can use the `currentRoute` property of Ember's router service to trigger changes when navigating between pages. A common `{{#each` trick is used to fully destroy/re-render components even if the ad network is unchanged.
This commit is contained in:
David Taylor 2023-11-07 21:12:30 +00:00 committed by GitHub
parent 5ad841de0b
commit c88bb59d81
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 35 additions and 85 deletions

View File

@ -1,12 +1,13 @@
{{#each adComponents as |adComponent|}}
{{component
adComponent
placement=placement
refreshOnChange=refreshOnChange
category=category
listLoading=listLoading
postNumber=postNumber
indexNumber=indexNumber
tagName=childTagName
}}
{{! Trick to force full destroy/re-render of component when route changes }}
{{#each (array this.router.currentRoute)}}
{{component
adComponent
placement=placement
category=category
postNumber=postNumber
indexNumber=indexNumber
tagName=childTagName
}}
{{/each}}
{{/each}}

View File

@ -1,6 +1,7 @@
import EmberObject from "@ember/object";
import { inject as service } from "@ember/service";
import { isBlank } from "@ember/utils";
import discourseComputed, { observes } from "discourse-common/utils/decorators";
import discourseComputed from "discourse-common/utils/decorators";
import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
import {
isNthPost,
@ -173,7 +174,7 @@ export function slotContenders(
}
export default AdComponent.extend({
needsUpdate: false,
router: service(),
tagName: "",
/**
@ -191,27 +192,14 @@ export default AdComponent.extend({
);
},
/**
* When house ads are configured to alternate with other ad networks, we
* need to trigger an update of which ad component is shown after
* navigating between topic lists or topics.
*/
@observes("refreshOnChange")
changed() {
if (this.get("listLoading")) {
return;
}
// force adComponents to be recomputed
this.notifyPropertyChange("needsUpdate");
},
/**
* Returns a list of the names of ad components that should be rendered
* in the given ad placement. It handles alternating between house ads
* and other ad networks.
*
* Depends on `router.currentRoute` so that we refresh ads when navigating around.
*/
@discourseComputed("placement", "availableAdTypes", "needsUpdate")
@discourseComputed("placement", "availableAdTypes", "router.currentRoute")
adComponents(placement, availableAdTypes) {
if (
!availableAdTypes.includes("house-ad") ||

View File

@ -2,7 +2,7 @@ import { scheduleOnce } from "@ember/runloop";
import RSVP from "rsvp";
import loadScript from "discourse/lib/load-script";
import { isTesting } from "discourse-common/config/environment";
import discourseComputed, { observes } from "discourse-common/utils/decorators";
import discourseComputed from "discourse-common/utils/decorators";
import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
let _loaded = false,
@ -111,16 +111,6 @@ export default AdComponent.extend({
scheduleOnce("afterRender", this, this._triggerAds);
},
@observes("listLoading")
waitForLoad() {
if (this.get("adRequested")) {
return;
} // already requested that this ad unit be populated
if (!this.get("listLoading")) {
scheduleOnce("afterRender", this, this._triggerAds);
}
},
@discourseComputed("currentUser.trust_level")
showToTrustLevel(trustLevel) {
return !(

View File

@ -3,7 +3,7 @@ import { htmlSafe } from "@ember/template";
import RSVP from "rsvp";
import loadScript from "discourse/lib/load-script";
import { isTesting } from "discourse-common/config/environment";
import discourseComputed, { observes } from "discourse-common/utils/decorators";
import discourseComputed from "discourse-common/utils/decorators";
import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
let _loaded = false,
@ -163,23 +163,9 @@ export default AdComponent.extend({
return;
}
if (this.get("listLoading")) {
return;
}
scheduleOnce("afterRender", this, this._triggerAds);
},
@observes("listLoading")
waitForLoad() {
if (this.get("adRequested")) {
return;
} // already requested that this ad unit be populated
if (!this.get("listLoading")) {
scheduleOnce("afterRender", this, this._triggerAds);
}
},
@discourseComputed("ad_width")
isResponsive(adWidth) {
return ["auto", "fluid"].includes(adWidth);

View File

@ -242,7 +242,6 @@ export default AdComponent.extend({
classNameBindings: ["adUnitClass"],
classNames: ["google-dfp-ad"],
loadedGoogletag: false,
refreshOnChange: null,
lastAdRefresh: null,
width: alias("size.width"),
height: alias("size.height"),
@ -353,7 +352,7 @@ export default AdComponent.extend({
@on("didUpdate")
updated() {
if (this.get("listLoading") || !this.shouldRefreshAd()) {
if (!this.shouldRefreshAd()) {
return;
}

View File

@ -1,5 +1,5 @@
import { isBlank } from "@ember/utils";
import discourseComputed, { observes } from "discourse-common/utils/decorators";
import discourseComputed from "discourse-common/utils/decorators";
import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
const adIndex = {
@ -100,12 +100,7 @@ export default AdComponent.extend({
}
},
@observes("refreshOnChange")
refreshAd() {
if (this.get("listLoading")) {
return;
}
this.set("adHtml", this.chooseAdHtml());
},
@ -116,10 +111,6 @@ export default AdComponent.extend({
return;
}
if (this.get("listLoading")) {
return;
}
if (adIndex.topic_list_top === null) {
// start at a random spot in the ad inventory
Object.keys(adIndex).forEach((placement) => {

View File

@ -1,8 +1,6 @@
{{ad-slot
placement="topic-list-between"
refreshOnChange=listLoading
category=category.slug
listLoading=listLoading
indexNumber=index
childTagName="td"
}}

View File

@ -1,6 +1 @@
{{ad-slot
placement="topic-list-top"
refreshOnChange=listLoading
category=category.slug
listLoading=listLoading
}}
{{ad-slot placement="topic-list-top" category=category.slug}}

View File

@ -1,5 +1 @@
{{ad-slot
placement="topic-above-post-stream"
refreshOnChange=model.id
category=model.category.slug
}}
{{ad-slot placement="topic-above-post-stream" category=model.category.slug}}

View File

@ -1,5 +1 @@
{{ad-slot
placement="topic-above-suggested"
refreshOnChange=model.id
category=model.category.slug
}}
{{ad-slot placement="topic-above-suggested" category=model.category.slug}}

View File

@ -2,6 +2,7 @@ import { visit } from "@ember/test-helpers";
import { test } from "qunit";
import {
acceptance,
query,
updateCurrentUser,
} from "discourse/tests/helpers/qunit-helpers";
@ -69,12 +70,21 @@ acceptance("House Ads", function (needs) {
assert
.dom(".h-topic-list")
.exists({ count: 1 }, "it should render ad above topic list");
const originalTopAdElement = query(".h-topic-list");
await visit("/latest");
assert
.dom(".h-between-topic-list")
.exists({ count: 5 }, "it should render 5 ads between topics");
await visit("/top");
const newTopAdElement = query(".h-topic-list");
assert.notStrictEqual(
originalTopAdElement,
newTopAdElement,
"ad is fully re-rendered when changing pages"
);
await visit("/t/28830");
assert