-
Notifications
You must be signed in to change notification settings - Fork 798
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
Subscription Block: Add success message #21273
Subscription Block: Add success message #21273
Conversation
…ption-widget-transform
In this commit, I have updated all the files of subscription block extension I have also updated the modules views.php file to support success_message as a shortcode attribute.
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. Jetpack plugin:
|
Thanks for the contribution @amustaque97 ! Is this PR still in progress or ready for review? |
It is ready for review |
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 works for me! I have a few in line comments that I think we can tighten up. I'm also not an expert on block or react, so if someone else would like to take a look to make sure I'm not missing anything, that'd be appreciated too!
projects/plugins/jetpack/extensions/blocks/subscriptions/controls.js
Outdated
Show resolved
Hide resolved
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.
Nice work! You'll now want to update the tests for the block to reflect your changes. You can check the projects/plugins/jetpack/extensions/blocks/subscriptions/test/
to find out more, and run cd projects/plugins/jetpack && pnpm test-extensions
to run those tests locally.
@jeherve, Thank you for the appreciation 🙌 and apologies for the delay 😞. I have fixed all the test cases. Please have a look again. All test cases are passing 😄 |
Thank you. Could you run |
Took a merge from the master branch. Test cases are passing. |
Changes addressed
…ock-success-message
projects/plugins/jetpack/extensions/blocks/subscriptions/deprecated/v6/save.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/blocks/subscriptions/deprecated/v6/save.js
Outdated
Show resolved
Hide resolved
...ts/plugins/jetpack/extensions/blocks/subscriptions/test/fixtures/jetpack__subscriptions.html
Show resolved
Hide resolved
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.
Looking good @amustaque97 ! just some cleanup in v6/save.js
is needed and I would try redoing the fixtures once that is completed 👍
On the sidebar of this PR I think there should be an option to let Jetpack maintainers write to your branch if you don't mind so we don't keep having to ask for a master merge 😉
Co-authored-by: Samiff <samiff@users.noreply.github.com>
…ock-success-message
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.
Those last changes look good, and the tests are passing from what I see 👍
Master will need to be merged once more to bring this up to date.
…ock-success-message
Thank you @jeherve and @samiff for helping me with this. I can see this PR took long than expected, more than 2 months to be exact 😅 . Thanks once again for helping me out with this and patiently reviewing my every single commit and multiple code reviews. ❤️ Please consider/ping me in other issues/PR related to deprecation implementation. Would love to get involved. 🚀 |
Thanks for working on this @amustaque97 🎉 This change will ship out in Jetpack version 10.5 which is scheduled to release on: 2022-01-11 Internal: D71163-code marked for 10.5 release (sync with wpcom release) |
* master: (24 commits) Replace base styles with component driven approach (#21938) Release Unreleased GH Actions, JS and Composer Packages (#22041) Have eslint and phpcs ignore jetpack_vendor (#22056) Update PHP 7.4 to 8.0 in our tools (#22046) Connection: Handle package versions on site registration (#22025) Backup plugin: Add the jetpack-identity-crisis package to composer.json (#22036) IDC UI: update the default text in the migrate card (#22033) Doc PHPCS updates (#22042) docker: Fix mu-plugins/fix-monorepo-plugins-url.php (#22044) E2E search plan data fixture (#22039) Composer Packages: change the package type to jetpack-library (#21934) Subscription Block: Add success message (#21273) My Jetpack: do not ship scss and jsx files in production build. (#22032) Notes: Remove old code (#22026) SSO: Add filters for error text (#22004) Backport 10.4 Changelog (#22034) Releases: start 10.5-a.2 cycle (#22031) Releases: prepare changelogs for Jetpack version 10.5-a.1 (#22030) Monorepo now uses Composer 2.1 (#22028) Notifications: PHPCS (#22017) ...
Follow-up: #22193 seems to highlight an issue on WordPress.com Simple sites. |
Fixes #21187
Changes proposed in this Pull Request:
projects/plugins/jetpack/extensions/blocks/subscriptions
to support teaxtarea forsuccess_message
in the widget setttings.projects/plugins/jetpack/modules/subscriptions/views.php
to supportsuccess_message
when passed as an attribute to the shortcode.Jetpack product discussion NA
Does this pull request to change what data or activity we track or use? NA
Testing instructions:
Screenrecording https://youtu.be/Z5cSgh5neQA