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

fix: disable block if not using sidebars #99

Merged
merged 3 commits into from
Mar 11, 2022
Merged

Conversation

miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Mar 8, 2022

This PR disables the registration of the SCAIP block if the sidebars are disabled. SCAIP should assume that disabling sidebars means there is another method in place for handling how the ads are registered and rendered. There's no need for it to have its own block in such a case since it can duplicate and potentially conflict with the feature that has taken over.

To fix the integration with the block-based widget editor introduced by WordPress 5.8, this PR also removes the wp-editor dependency from the block editor JS. Declaring its dependency does not affect the block behavior.

How to test

Block-based widgets

  1. With a fresh installation running only this SCAIP branch, visit the widgets editor
  2. Confirm you are able to add a widget to the SCAIP sidebars and render it on the post

Disabled SCAIP block

  1. Running fix: enable block-based widgets newspack-plugin#1550 with Newspack Ads and this branch of SCAIP, visit Newspack Ads -> Settings
  2. Confirm you have legacy mode for SCAIP turned on
  3. Create a new page and confirm the block works as expected
  4. Visit the Settings again and turn legacy mode off
  5. Confirm you are no longer able to add the block to a page
  6. Repeat the steps from "block-based widgets" and confirm it also works as expected

Note: If you are running an instance with Jetpack enabled and connected, you might still see the warning due to Automattic/jetpack#20357

@miguelpeixe miguelpeixe changed the title fix: disable block if not using sidebars and remove dependency fix: disable block if not using sidebars Mar 8, 2022
@miguelpeixe miguelpeixe self-assigned this Mar 8, 2022
@miguelpeixe miguelpeixe marked this pull request as ready for review March 8, 2022 20:59
@miguelpeixe miguelpeixe requested a review from a team March 8, 2022 20:59
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Ran into a couple of issues:

Confirm you are no longer able to add the block to a page

This is true, but if you visit a post that previously had the block, you'll see this in its place:
Screen Shot 2022-03-09 at 10 27 47 AM

Clicking the button results in a 500 error:

PHP Fatal error:  Cannot redeclare scaip_insert_shortcode() (previously declared in /srv/users/user67f0c08d/apps/user67f0c08d/public/wp-content/plugins/super-cool-ad-inserter-plugin/inc/scaip-shortcode-inserter.php:16) in /srv/users/user67f0c08d/apps/user67f0c08d/public/wp-content/plugins/super-cool-ad-inserter/inc/scaip-shortcode-inserter.php on line 16

blocks/scaip-sidebar.php Outdated Show resolved Hide resolved
@miguelpeixe
Copy link
Member Author

Thank you for catching that! I realize now that we should have a better strategy for disabling the use of the block instead of removing its registration entirely.

07a223e displays an error message on block edit and modifies the render callback.

image

@miguelpeixe miguelpeixe requested a review from dkoo March 9, 2022 19:42
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

The placement of the error notice is a little strange, but this works for me!

@miguelpeixe miguelpeixe merged commit 30ef844 into master Mar 11, 2022
@miguelpeixe miguelpeixe deleted the fix/sidebar-block branch March 11, 2022 14:00
matticbot pushed a commit that referenced this pull request Mar 15, 2022
## [0.6.1-alpha.1](v0.6.0...v0.6.1-alpha.1) (2022-03-15)

### Bug Fixes

* disable block if not using sidebars ([#99](#99)) ([30ef844](30ef844))
* syntax error on sample code ([#98](#98)) ([57a1133](57a1133))
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Mar 22, 2022
## [0.6.1](v0.6.0...v0.6.1) (2022-03-22)

### Bug Fixes

* disable block if not using sidebars ([#99](#99)) ([30ef844](30ef844))
* syntax error on sample code ([#98](#98)) ([57a1133](57a1133))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 0.6.1 🎉

The release is available on GitHub release

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.

3 participants