-
Notifications
You must be signed in to change notification settings - Fork 808
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
Decouple subscription block from Jetpack's subscription widget shortcode #26947
Decouple subscription block from Jetpack's subscription widget shortcode #26947
Conversation
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. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run |
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.
Tested well for me. I left a comment regarding "weight/width" as variable name, but that's no blocker (if you decide to change it, I'll approve again).
There's this behavior after submit where the page would scroll to the top of the form, but I think we can look at it on a separate iteration.
* These block defaults should match ./constants.js | ||
*/ | ||
const DEFAULT_BORDER_RADIUS_VALUE = 0; | ||
const DEFAULT_BORDER_WEIGHT_VALUE = 1; |
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.
Is this actually "weight"? Should it be "width"?
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 doesn't really matter since it's just a PHP constant, but the css prop is indeed border-width
.
As a good part of this code was migrated from the shortcode implementation, I opted not to make too many changes for now and left this one as is.
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.
Works for me, looks good!
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.
Hi there,
I tried testing this using the provided instructions and I noticed the following unexpected behaviours:
- After editing the form by changing the button background, border radius etc the changes don't seem to apply (see attached screenshots)
- I get the following warning:
Warning: sprintf(): Too few arguments in /srv/users/usercf4c29e8/apps/usercf4c29e8/public/wp-content/plugins/jetpack-dev/extensions/blocks/subscriptions/subscriptions.php on line 189
@fgiannar The issues should be fixed now, thanks for catching it :) |
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.
LGTM
@ice9js tested this in Jetpack and .com and I still see the block saving the shortcode |
That’s correct. This PR made the block dynamic which effectively means the shortcode is ignored, but it is still being saved. |
Fixes #26866.
Changes proposed in this Pull Request:
This is an internal PR which solves the issue of the subscription block being closely dependent on Jetpack Subscription Widget for rendering the form - resulting in less flexibility and more difficult development process with the need of taking both into account.
In order to accomplish this, the subscribe block is now a dynamic block, employing
render_block()
to render it from the backend/PHP instead of relying on the block editor'ssave()
method.While there's still some dependency on the
Jetpack_Form_Widget
class for form handling purposes, the view itself is now completely independent from the shortcode/widget.I think this is a good first step, and now that it's done it'll probably allow for eventual optimization and simplification of the block code, but I'd argue that's outside of the scope of this PR itself.
Other information:
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
This PR is meant to be an internal change only, hence the best way of testing is side by side on two separate sites with the current and the new implementation and compare.