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

feat: homepage posts patterns with ad unit #1170

Merged
merged 5 commits into from
Jul 7, 2022
Merged

Conversation

miguelpeixe
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

Updates Homepage Posts patterns 2, 7, 14, 26 and implements new pattern 31 as described in Automattic/newspack-ads#210.

This PR also inaugurates "placeholder blocks", a simple config to allow a more descriptive missing block. The placeholder should provide an easy button to remove the missing block, a description and a link to the plugin that provides the block:

image

Closes Automattic/newspack-ads#210

How to test the changes in this Pull Request:

  1. With Newspack Ads active, create a new page and add patterns listed above
  2. Confirm they reflect Add ad blocks to Newspack block patterns newspack-ads#210
  3. Deactivate Newspack Ads
  4. Refresh the editor
  5. Confirm the Ad Unit blocks are replaced with the placeholder as pictured above

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@miguelpeixe miguelpeixe requested a review from a team as a code owner June 14, 2022 19:10
@miguelpeixe miguelpeixe self-assigned this Jun 14, 2022
@thomasguillot thomasguillot self-requested a review June 15, 2022 08:34
@thomasguillot
Copy link
Contributor

@miguelpeixe this is great!

I pushed some visual changes to make it look like more of a placeholder.

One thing I was thinking, the "Visit Plugin Page" button doesn't add much value here. It's potentially distracting the user if they click on it, losing their tab/editor just to see our Github page. Instead, would it be possible to Install/Activate the plugin directly from here? I know they would still need to setup Newspack Ads, etc... but at least they'd have the plugin already active.

@thomasguillot thomasguillot removed their request for review June 15, 2022 10:25
@miguelpeixe
Copy link
Member Author

Thank you, @thomasguillot! It's looking so much better now!

One thing I was thinking, the "Visit Plugin Page" button doesn't add much value here.

I thought about it and the issue with Ads is not only that you currently can't just one-click install and use it inside the block editor but also that Newspack Ads depends on Newspack Plugin. We'd have to install 2 plugins on this button action and I really don't think we should.

The link, which opens in a new window, at least suggests where to find the missing feature. I believe it's better than nothing, considering the one-click install is not feasible at this moment.

@miguelpeixe
Copy link
Member Author

Related, for future implementation of a one-click solution: Automattic/newspack-ads#444

@thomasguillot
Copy link
Contributor

I thought about it and the issue with Ads is not only that you currently can't just one-click install and use it inside the block editor but also that Newspack Ads depends on Newspack Plugin. We'd have to install 2 plugins on this button action and I really don't think we should.

Is there any way for us to check if Newspack Plugin is active and Newspack Ads is simply inactive? Then we should the "Activate Plugin" button.
And if not, we display the "Visit Plugin Page" button

@miguelpeixe
Copy link
Member Author

Is there any way for us to check if Newspack Plugin is active and Newspack Ads is simply inactive? Then we should the "Activate Plugin" button.
And if not, we display the "Visit Plugin Page" button

It is a possibility, but I'd rather tackle it separately as it also depends on Automattic/newspack-ads#444

description: __( 'Render an ad unit from your inventory.', 'newspack-blocks' ),
icon: pullquote,
message: __( 'Place ad units inside your page by installing Newspack Ads.', 'newspack-blocks' ),
url: 'https://github.com/Automattic/newspack-ads',
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Github URL won't be of much use to most WP users, might be better to point to https://newspack.pub/support/advertising/

@miguelpeixe miguelpeixe merged commit a2e652e into master Jul 7, 2022
@miguelpeixe miguelpeixe deleted the feat/ad-unit-patterns branch July 7, 2022 14:30
matticbot pushed a commit that referenced this pull request Jul 14, 2022
# [1.54.0-alpha.1](v1.53.1...v1.54.0-alpha.1) (2022-07-14)

### Bug Fixes

* resolve merge conflict with release ([#1199](#1199)) ([ce82826](ce82826))

### Features

* **donate-block:** amounts and frequencies customisation ([#1191](#1191)) ([99d967f](99d967f))
* homepage posts patterns with ad unit ([#1170](#1170)) ([a2e652e](a2e652e))
* support new sponsor options to show authors and categories ([#1156](#1156)) ([67e8834](67e8834))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.54.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Jul 26, 2022
# [1.54.0](v1.53.1...v1.54.0) (2022-07-26)

### Bug Fixes

* resolve merge conflict with release ([#1199](#1199)) ([ce82826](ce82826))

### Features

* **donate-block:** amounts and frequencies customisation ([#1191](#1191)) ([99d967f](99d967f))
* homepage posts patterns with ad unit ([#1170](#1170)) ([a2e652e](a2e652e))
* support new sponsor options to show authors and categories ([#1156](#1156)) ([67e8834](67e8834))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.54.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Add ad blocks to Newspack block patterns
4 participants