-
Notifications
You must be signed in to change notification settings - Fork 846
Form Block: add a new Consent Field, a new Newsletter setting, and a new newsletter variation #16808
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
Conversation
|
Caution: This PR has changes that must be merged to WordPress.com |
|
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-16808 Scheduled Jetpack release: September 1, 2020. |
jeherve
left a comment
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.
extensions/blocks/contact-form/components/jetpack-newsletter-integration-settings.js
Outdated
Show resolved
Hide resolved
Good catch. I logged this here: #16822 In our case though, we'd still want to handle the text changes for that field. A change like this: should be reflected on the frontend but it's not at the moment: |
|
Hello, @jeherve, thanks for the ping! I went through the process of creating a new post and adding the newsletter sign-up element to it. Here are some thoughts I had on copy as I went through the process. In the first step, I'm presented with a set of options from which to make a selection: In the next step, I'm presented with the draft form: I have a small suggestion to tighten the disclaimer copy: Original:
Suggested:
I'm not sure if the Form Settings copy can be customized for the Newsletter Sign-up, though here are some thoughts: The descriptor for a regular form, "A simple way to get feedback from folks visiting your site" might be a little confusing within this context. Suggest:
If it's not possible to customize to this degree, another option which fits this scenario is:
For "Email address to send to" within this context, how about: Suggest:
For "A new message from your website" Suggest:
Thanks again for asking us to have a look! If you have any questions, please let me know! |
.....
updated the PR: I'm afraid that changing the description of the "form" is a generic text for every form variation. Thanks for the feedback! |
jeherve
left a comment
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 well overall! 👍 I only have some minor remarks left.
extensions/blocks/contact-form/components/jetpack-field-consent.js
Outdated
Show resolved
Hide resolved
jeherve
left a comment
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 good to me, I think it should be good to merge now!
I can't approve that PR myself since I opened it, so I'll let someone else on the team take a stab at it.
Once this first iteration is merged, and once #16713 is also merged, we should be able to iterate on the settings panel to allow installing the plugin straight from the editor, instead of just linking to it.
I can't reproduce it. Every time I export the CSV I'm not able to see the consent field. I've tried different form types and both with the checkbox or just the message. Here are 2 examples: |
|
@leogermani i just pushed a commit fixing the CSV export |
Thanks, it works now! As a suggestion, I'd just say it would be awesome to have the Consent information on the Feedback listing in the admin panel. I can approve the PR as soon as all the tests are passing... I think first thing to do is to update it against master. |
|
Looks like this test will need to be updated: |
|
The unit tests are fixed, And I notice that the codeclimate/diff-coverage — 10% (80% threshold) is not passing yet. It looks like it's not passing on the code that was already created a long time ago, do you have any suggestions for this? |
That is something we can ignore in this case, since we're touching existing code. |
leogermani
left a comment
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.
So it looks like we are good to go! :)
|
A bit late but here's how my testing went:
|
|
r212360-wpcom |
* master: (23 commits) Premium Blocks: set blocks availability (#16898) Compat Package: Fix method declaration compatibility (#16900) Jetpack Dashboard: More meaningful error notices. (#16883) Connection REST API: Unit test for the `remote_authorize` request. (#16879) use blog token to request jetpack.updateBlog (#16698) Improve Story block media loading (#16663) Simplify error notices for broken connections (#16655) Use new heartbeat package (#16285) wrap-paid-block: remove component. deprecated. (#16895) Social Previews: improve preview description handling (#16889) Stats module use blog token (#16727) Form Block: add a new Consent Field, a new Newsletter setting, and a new newsletter variation (#16808) AAG: Backup card, fall back to VP content in case of /rewind API error. (#16867) Donations: Fix dependencies (#16892) Creative Mail: update option to lowercase (#16861) Premium Blocks: Implement the new design (#16611) Requests to Stats CSV use the blog token (#16716) Update spacing around sharing buttons to avoid no bottom margin below the customize link. (#16811) Jetpack SSO: Cleaning up the `requestNonce` API request. (#16830) Donations: Update plans when currency changes (#16844) ...
…setting, and a new newsletter variation (#16808) Co-authored-by: Reint-Jan Hoiting <[email protected]>



















This is a replay of #16723 against
masteras we're running into some rebase problems with the other branch.Changes proposed in this Pull Request:
This PR extends the form block with some new settings and options:
To do
In follow-up PRs, there are a few additional things we can do:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
test case 1
test case 2
/Todo WIP/
/Todo WIP/
test case 3
/Todo WIP/
/Todo WIP/
Proposed changelog entry for your changes: