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

Decouple subscription block from Jetpack's subscription widget shortcode #26947

Merged
merged 7 commits into from
Nov 18, 2022

Conversation

ice9js
Copy link
Member

@ice9js ice9js commented Oct 19, 2022

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's save() 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.

Screenshot 2022-10-19 at 22 28 18

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?

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.

  • Make sure subscriptions are enabled on your site (Jetpack->Discussion->Subscriptions). You will also need the connection to Jetpack/WP.com to be available during the testing.
  • Create or edit a post and add a Subscription block.
  • When you publish, the subscription block display normally on the page.
  • You should be able to submit the form using a correct email address, after which a success message will be shown and you'll receive a confirmation email.
  • When you submit an invalid email, you should see an error.
  • Try the various style setting of the block, including displaying the button on a new line or adding the subscriber count. Everything should work as it does in the existing version.

@ice9js ice9js self-assigned this Oct 19, 2022
@github-actions github-actions bot added [Block] Subscriptions [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Oct 19, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2022

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: December 6, 2022.
  • Scheduled code freeze: November 28, 2022.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2022

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run bin/jetpack-downloader test jetpack update/decouple-substriptions-block-from-shortcode to get started. More details: p9dueE-5Nn-p2

@donnchawp donnchawp self-requested a review October 26, 2022 14:18
CGastrell
CGastrell previously approved these changes Oct 26, 2022
Copy link
Contributor

@CGastrell CGastrell left a 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;
Copy link
Contributor

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"?

Copy link
Member Author

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.

@ice9js ice9js added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Team Review labels Oct 26, 2022
donnchawp
donnchawp previously approved these changes Oct 27, 2022
Copy link
Contributor

@donnchawp donnchawp left a 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!

Copy link
Contributor

@fgiannar fgiannar left a 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:

  1. After editing the form by changing the button background, border radius etc the changes don't seem to apply (see attached screenshots)
  2. 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

Screenshot 2022-11-02 at 09 36 53

Screenshot 2022-11-02 at 09 34 20

@fgiannar fgiannar added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 2, 2022
@ice9js ice9js dismissed stale reviews from donnchawp and CGastrell via 6a0885b November 16, 2022 13:42
@ice9js
Copy link
Member Author

ice9js commented Nov 16, 2022

@fgiannar The issues should be fixed now, thanks for catching it :)

@ice9js ice9js added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 16, 2022
Copy link
Contributor

@bindlegirl bindlegirl left a comment

Choose a reason for hiding this comment

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

LGTM

@bindlegirl bindlegirl added the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 17, 2022
@bindlegirl bindlegirl removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Nov 17, 2022
@bindlegirl bindlegirl added this to the jetpack/11.6 milestone Nov 17, 2022
@ice9js ice9js merged commit 6266a57 into trunk Nov 18, 2022
@ice9js ice9js deleted the update/decouple-substriptions-block-from-shortcode branch November 18, 2022 19:01
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 18, 2022
@oskosk
Copy link
Contributor

oskosk commented Nov 18, 2022

@ice9js tested this in Jetpack and .com and I still see the block saving the shortcode

image

@ice9js
Copy link
Member Author

ice9js commented Nov 19, 2022

@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.
My reasoning was: one step at a time. I’ll be adding another PR which removes the current save() implementation from the block entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Subscriptions [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jetpack: Subscriptions - Decouple shortcode from Block rendering
6 participants