-
Notifications
You must be signed in to change notification settings - Fork 800
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
Subscriptions: stop loading subscribe block and panels when module disabled #39802
Subscriptions: stop loading subscribe block and panels when module disabled #39802
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
This is also nice, @simison 🚀 |
* Do not proceed if the newsletter feature is not enabled | ||
* or if the 'Jetpack_Memberships' class does not exists. | ||
*/ | ||
if ( ! class_exists( '\Jetpack_Memberships' ) ) { |
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.
It was a bit weird to see this class check here since we just import the file a few lines above:
jetpack/projects/plugins/jetpack/extensions/blocks/subscriptions/subscriptions.php
Line 46 in adb410b
require_once JETPACK__PLUGIN_DIR . '/modules/memberships/class-jetpack-memberships.php'; |
So I removed the check as a redundant.
|
||
if ( \Jetpack_Memberships::should_enable_monetize_blocks_in_editor() ) { |
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.
Function name could be improved but works for now.
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 essentially removes the work we had done in #29044. This introduces a discrepancy with the other blocks that rely on a module, and that will remain always available. is that okay?
Could we instead hide the newsletter icon, but keep the placeholders to invite folks to turn the feature on?
It's an option, yes. I'm little less worried about undoing work (although it's a bummer) and more interested in finding best possible UX. :-) Happy to have the product convo in a P2 regarding other blocks, before committing to this! |
It may be worth a discussion on +jpopdesignp2 I think. I don't have a strong opinion on the matter personally, but I think it would make sense to have a consistent approach. If we were to remove the placeholders for one block, we should probably do it for all. I think there are arguments for it, performance could be one of them. |
My thoughts: When a user opens Gutenberg, they're usually focused on writing content for their site, not exploring new features. Sure, they could come across new features, but that's not the main goal for this UI. I think we can find better opportunities to introduce features within the WP Admin or Jetpack, rather than adding more icons to Gutenberg that users may not need right away (or maybe never). I'm not entirely sure we have a one-size-fits-all approach, it probably depends on the feature. AI, for example, directly helps with writing, so that one makes sense to keep. |
1501f5e
to
eec3ab3
Compare
eec3ab3
to
22f8a36
Compare
@jeherve I was reviewing a bit placehodler status for other features: While Newsletters feature has a clear'ish "on/off" toggle (it could be better discoverable but that's separate issue), not all features do have it: For example Related posts has a toggle which adds classic related posts to posts: Having that toggle control the block is kinda meaningless since related posts would just appear in your posts, you don't need to deal with the block anymore. Once related posts is updated to use block hooks, hiding the block when module is disabled is more meaningful. There are likely similar UX issues with other blocks, and placeholders are kinda "hack" to improve otherwise poor UX for enabling and discovering features, but have a tradeoff of contributing to the bloated feeling of Jetpack. |
Yup, that's very true. Interestingly, the concept of modules also contributed to fight the idea of bloat since when a module is inactive, code for that feature didn't run on the site. |
Subscriptions: stops loading editor functionality (i.e. the block and panels) when subscriptions module is disabled.
Resolves #39800
Resolves #40230
Proposed changes:
Newsletter in the header icon will be no more, and the subscribe block won't show up in block pickers:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Block and panels work when subscriptions module is enabled from "Jetpack -> Settings -> Newsletter"
You won't find any functionality in the editor when module is disabled: