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.

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

WordAds: Include Inline Ads Setting #91006

Merged
merged 4 commits into from
May 22, 2024

Conversation

Aurorum
Copy link
Contributor

@Aurorum Aurorum commented May 22, 2024

Fixes #90997

Proposed Changes

Adds the "Inline within post content" settings to the WordAds dashboard on Calypso.

Why are these changes being made?

These changes were already recently added to Jetpack in WP-Admin: Automattic/jetpack#37170. This PR brings parity with Calypso.

Testing Instructions

  1. Visit /earn/ads-settings/ and confirm that the setting is listed there
  2. Toggle the setting
  3. Visit /wp-admin/admin.php?page=jetpack#/earn, and confirm that the toggle updates to match Calypso.
  4. You can also toggle the setting in WP-Admin and ensure that it is reflected upon refreshing Calypso.

@cphilleo
Copy link
Contributor

Coincidentally, I was working on this same task and was ready to push a PR almost identical to yours. The only difference was that I also included the option for non-Jetpack sites—since they should be able to toggle this as well.

We can use your PR with a small update. Could you update your supportsInlineAds check to be either 1) a simple site or 2) a Jetpack site with the required minimum version?

@Aurorum
Copy link
Contributor Author

Aurorum commented May 22, 2024

Whoops, sorry for the overlap! :) Ah, good point, completely missed that - should be fixed now.

Copy link
Contributor

@cphilleo cphilleo left a comment

Choose a reason for hiding this comment

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

Tests great! Thanks for working on this. LGTM.

@cphilleo cphilleo merged commit 32b2cc2 into Automattic:trunk May 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ads Dashboard missing additional ad placement
2 participants