WordAds: Add inline ads within post content #37170
Conversation
github-actions
bot
added
[Feature] Ad
[Package] Sync
[Plugin] Jetpack
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Migration plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Automattic For agencies client plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
There was a problem hiding this comment.
Great job here! I've added minor comments.
Working as expected, I was able to see the inline ads injected between the post paragraphs.
Turning the placement off I got no logs or adflow requests in Network tab as expected:
Note: In order to see the inline ad feature under the Monetize section I had to build wordads package and the jetpack plugin again, jetpack build.
projects/plugins/jetpack/modules/wordads/php/class-wordads-smart.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/modules/wordads/php/class-wordads-smart.php
Outdated
Show resolved
Hide resolved
|
LGTM! This works as expected. I was able to see inline ads Not sure if this is related to the PR but I saw an error in the console a couple of times before the ads started showing 
|
github-actions
bot
added
[Plugin] Automattic For Agencies Client
[Plugin] Backup
| * | ||
| * @return string The URL. | ||
| */ | ||
| private function get_config_url(): string { |
There was a problem hiding this comment.
Jetpack still requires PHP 7.0, so I'm afraid we won't be able to rely on type declarations just yet.
https://github.com/Automattic/jetpack/blob/trunk/.github/versions.sh#L8
There was a problem hiding this comment.
This one is ok in PHP 7.0. https://www.php.net/manual/en/language.types.declarations.php has decent coverage of which things were added in later versions.
| * | ||
| * @return void | ||
| */ | ||
| public function init( WordAds_Params $params ) { |
There was a problem hiding this comment.
I'm afraid type definitions are not an option yet in the Jetpack codebase, since we still require PHP 7.0 for now.
There was a problem hiding this comment.
A class type on a parameter like this is ok even back to PHP 5.0.
| return; | ||
| } | ||
|
|
||
| // Don't run on not found pages. |
There was a problem hiding this comment.
Do we need to ensure those don't get added to RSS feeds and embeds too, where JS will not be available?
There was a problem hiding this comment.
For now, I'm going to stick with the same checks that our existing ads are using, but I'll look into this further. The side effect if no JS is available would be an extra invisible <span> at the end of the post content—so not too disruptive.
jeherve
added
[Status] Needs Author Reply
cphilleo
force-pushed
the
add/wordads-blaze
branch
from
5ea9fef
to
d7fe574
Compare
cphilleo
added
[Status] Needs Review
|
@jeherve, I'm pretty confident in the changes I made for this patch, but I'm less confident in the changes that are here because of having to run |
These are safe. 👍 The script automatically bumps versions of packages that will include your changes, and updates the lock files accordingly. |
cphilleo
force-pushed
the
add/wordads-blaze
branch
from
d7fe574
to
8f55e83
Compare
github-actions
bot
removed
the
[Status] Needs Review
* Add support for inline Blaze ads with IPONWEB fallback * changelog * Minor fixes * Run fixup-project-versions.sh * Make WordAds_Smart a singleton
|
Hi all, there's been an issue opened in Automattic/wp-calypso#90997 about these settings not existing in the Calypso ads dashboard. I couldn't find any PR related to this on the Calypso side, so I've proposed bringing parity between this addition and Calypso in Automattic/wp-calypso#91006. Would be very grateful if you wouldn't mind reviewing it! :) |
Proposed changes:
Does this pull request change what data or activity we track or use?
No
Testing instructions:
wordads-logging=trueto the URL to enable detailed logging in the browser console