-
Notifications
You must be signed in to change notification settings - Fork 799
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
Allow pure Gutenberg extensions to be registered with Jetpack (with plan gate). #16804
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16804 Scheduled Jetpack release: September 1, 2020. |
Let's try and land #16746 first and then rebase this branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good first step towards the refactor suggested in #16816. Sorry about the nitpick comments.
I've just come back from AFK and have seen some activity around this gating discussion, so there's a good chance this PR is obselete now anyway!
|
||
// Attempt to register as Jetpack extension. | ||
if ( ! jetpack_register_extension( $slug, $args ) ) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and with the similar return statement below, I think we should be explicit about returning false
if ( isset( $args['version_requirements'] ) | ||
&& ! Jetpack_Gutenberg::is_gutenberg_version_available( $args['version_requirements'], $slug ) ) { | ||
return false; | ||
} | ||
|
||
// Checking whether block is registered to ensure it isn't registered twice. | ||
if ( Jetpack_Gutenberg::is_registered( $slug ) ) { | ||
return false; | ||
} | ||
|
||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably makes readability worse (and is almost the textbook definition of bikeshedding), but this could be rewritten to a single statement:
if ( isset( $args['version_requirements'] ) | |
&& ! Jetpack_Gutenberg::is_gutenberg_version_available( $args['version_requirements'], $slug ) ) { | |
return false; | |
} | |
// Checking whether block is registered to ensure it isn't registered twice. | |
if ( Jetpack_Gutenberg::is_registered( $slug ) ) { | |
return false; | |
} | |
return true; | |
return ! ( | |
( isset( $args['version_requirements'] ) && ! Jetpack_Gutenberg::is_gutenberg_version_available( $args['version_requirements'], $slug ) ) | |
// Checking whether block is registered to ensure it isn't registered twice. | |
&& Jetpack_Gutenberg::is_registered( $slug ) | |
); |
It's a personal preference thing but it seems odd to have if ( boolean check) { return false }
instead of return ! (boolean check)
* | ||
* @return Boolean Whether or not the registration was successful. | ||
*/ | ||
function jetpack_register_extension( $slug, $args = array() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does 'extension' only ever refer to Gutenberg extensions in Jetpack? I think that's the case, but otherwise, we might want to make that explicit here. jetpack_register_editor_extension
or something.
BLOCK_NAME, | ||
function register_extension() { | ||
jetpack_register_extension( | ||
EXTENSION_NAME, | ||
array( | ||
'plan_check' => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise it's not for this change, but the idea with plan_check
was that it was a way of opting into the plan check until we ensured that the features for each plan were set up correctly for all the blocks/extensions.
As this file only exists to register the extension it would be good to take this a step further, and blocks and extensions auto-registered, maybe using something similar to block.json?
This PR has been marked as stale. This happened because:
No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation. |
I'm not going to be picking this up so let's close it for now. |
Alternative approach to #16746.
Enables registering a Gutenberg extension with Jetpack without registering a block but including a plan check. New the API is:
Why do this?
Some extensions for the Gutenberg editor in Jetpack are not strictly blocks. However, there is no way to register an extension with Jetpack which is gated by a plan without either:
set_extension_available
.register_jetpack_block
.Moreover, if you need to gate your extension to a particular plan on either Jetpack or WPCom then you are forced to either:
register_jetpack_block
with theplan_check
argument - this is odd because the nomenclature ofextension
andblock
is jarring and confusing.set_extension_available
and perform the plan checks yourself manually.This PR attempts to normalise the situation by creating a new helper
register_jetpack_extension
which essential captures the functionality ofregister_jetpack_block
(including the ability to do plan checks), but avoids registering a block with Gutenberg viaregister_block
. The originalregister_jetpack_block
is left in place, but now simply delegates the bulk of its functionality to the aforementionedregister_jetpack_extension
, and then simply callsregister_block
if extension registration is successful.Use Cases
The
social-previews
extension needs to be gated for WPCom but not for Jetpack. Currently, in order to do this we are forced to register a block which doesn't do anything (because it isn't a block!). This is confusing because thesocial-previews
feature is not a block - it is purely adding to the Jetpack sidebar.Using the new approach in this PR we are able to register the
social-previews
extension and gate it to WPCom plans.Changes proposed in this Pull Request:
This change allows us to:
Jetpack product discussion
See 84-gh-dotcom-manage.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Same as #16746.
We should also check that all existing Jetpack blocks continue to be register "as was".
Proposed changelog entry for your changes: