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

Ads: January Ads Tweaks #8587

Merged
merged 2 commits into from
Jan 30, 2018
Merged

Ads: January Ads Tweaks #8587

merged 2 commits into from
Jan 30, 2018

Conversation

dbspringer
Copy link
Member

@dbspringer dbspringer commented Jan 22, 2018 •
edited
Loading

A handful of small updates to bring Jetpack Ads on par w/ .com.

Notably:

  1. Default header unit to on.
  2. Increment widget unit section ids so they’re unique.
  3. Ensure belowpost unit(s) only gets displayed once per page.

To Test

  1. Enable Jetpack Ads module on a fresh install
    1. Check that header unit is defaulted on
  2. Enable Display second ad below post
    1. Check to see that 2nd unit appears below post.
    2. Check to see that below post units only appear once in front page.
  3. Add two Ads widgets under Appearance -> Widgets, check both load.

@dbspringer dbspringer requested a review from a team as a code owner January 22, 2018 20:29
@dbspringer dbspringer added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] WordAds labels Jan 22, 2018
@dbspringer dbspringer added this to the 5.8 milestone Jan 22, 2018
@dbspringer dbspringer added [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 22, 2018
@dbspringer dbspringer changed the title Ads: Updates Ads: January Ads Tweaks Jan 22, 2018
zinigor
zinigor approved these changes Jan 23, 2018
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine, settings work fine as well, but for me when toggling the second ad below the post, they are shown in the same spot. Attaching the screenshot below.

@zinigor
Copy link
Member

zinigor commented Jan 23, 2018

screenshot from 2018-01-23 19-00-26

@dbspringer
Copy link
Member Author

Thanks for taking a look @zinigor, the double ad is as-intended. Once your site gets cleared in the back-end it'll eventually two different ads.

There might be a few more tweaks we need to make, I'll ping you if we end up needing another review.

@dbspringer dbspringer force-pushed the update/ads-updates-201801 branch from 08bb351 to 8cae65f Compare January 23, 2018 19:37
@dbspringer dbspringer added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress labels Jan 23, 2018
zinigor
zinigor reviewed Jan 29, 2018
View reviewed changes
modules/wordads/wordads.php Outdated

if ( $this->option( 'enable_header_ad' ) ) {
if ( ! apply_filters( 'wordads_content_disable', false ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nitpick - can you please add filter headers here? Here is an example of a comment block that will be picked up by the parser: https://github.com/Automattic/jetpack/blob/master/class.jetpack.php#L1552

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@dbspringer
A handful of ads updates for the next release
073339d 
Notably:
1. Default header unit to on
2. Increment widget unit section ids so they’re unique
3. Ensure belowpost unit(s) only gets displayed once per page

Check for non-main calls that might trigger the hooks

$belowpost_loaded is too problematic, adding filters to allow ad hoc disabling
@dbspringer dbspringer force-pushed the update/ads-updates-201801 branch from e690357 to 073339d Compare January 30, 2018 09:20
@dbspringer
Copy link
Member Author

No problem @zinigor, added.

jeherve
jeherve approved these changes Jan 30, 2018
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work for me too. Will merge when the tests complete.

@jeherve jeherve merged commit 28883f9 into master Jan 30, 2018
@jeherve jeherve deleted the update/ads-updates-201801 branch January 30, 2018 09:49
@jeherve jeherve added [Status] Has Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! [Status] Needs Changelog labels Jan 30, 2018
jeherve added a commit that referenced this pull request Jan 30, 2018
@jeherve
Changelog: add #8587
7fbc042
zinigor pushed a commit that referenced this pull request Jan 30, 2018
@oskosk @zinigor
Changelog 5.8: running changelog (#8442)
1ee2c03 
* Changelog 5.8: create base for changelog.

* Update 5.8 release post link

* fix 5.8 release date

* Updates to plugin description

* Changelog: add #8499

* Changelog: add #8506

* Changelog: add #8509

* Changelog: add #8516

* Changelog: add #8517

* Changelog: add #8523

* Changelog: add #8547

* Changelog: add #8496

* Changelog: add #8584

* Changelog: add #8595

* Changelog: add #8445

* Changelog: add #8431

* Changelog: add #8284

* Changelog: add #8270

* Changelog: add #8124

* Changelog: add #8581

* Changelog: add #8463

* Changelog: add #8568 (#8646)

* Updates to testing list and changelog

* Changelog: add #8443

* Changelog: add #8459

* Changelog: add #8469

* Changelog: add #8464

* Changelog: add #8478 and #8479

* Changelog: add #8483

* Changelog: add #8488

* Changelog: add #8513

* Changelog: add #8555

* Changelog: add #8565

* Changelog: add #8601

* Changelog: add #8612

* Changelog: add first pass at Search items.

* Changelog: add more info to help test Search.

* Changelog: add #8144

* Changelog: add #8313

* Changelog: add #8419

* Changelog: add #8465

* Changelog: add #8515

* Changelog: add #8587

* Changelog: add #8591

* Changelog: add #8659

* Changelog: add #8661

* Changelog: add #8671

* Changelog: add 5.7.1 to archived changelog too.

* Reverted changes to readme, removed entry about backups.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zinigor zinigor zinigor approved these changes

@rclations rclations rclations approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@dbspringer dbspringer

@rclations rclations

Labels
[Feature] WordAds
Projects
None yet
Milestone
5.8
Development

Successfully merging this pull request may close these issues.
5 participants
@dbspringer @zinigor @jeherve @rclations @matticbot