-
Notifications
You must be signed in to change notification settings - Fork 805
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
Mailchimp Block: Interests/SIGNUP merge field/AMP state hook #14050
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 Scheduled Jetpack release: December 3, 2019. |
jeffersonrabb, Your synced wpcom patch D35555-code has been updated. |
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.
I gave it a quick test, and I have some questions. It seems to work well for me overall, and it doesn't bring existing forms either.
In "Mailchimp Groups" select one or more groups.
Should I be able to do that when I set it as "Dropdown / can only select one at a time" when I created the groups?
Verify the email appears on the list in the Mailchimp dashboard and that it is in the correct groups and has the correct SIGNUP value.
I could find the group info, but not the SIGNUP value. I am not sure where to check that when looking at my subscriber list.
_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-mailchimp.php
Outdated
Show resolved
Hide resolved
@jeherve quick confirmation - in the above, "bring" === "break"? |
I believe I missed a step here. The field has to be defined in Mailchimp following these instructions: https://mailchimp.com/help/determine-webpage-signup-location/ When set up successfully you should see the values in the subscriber list as in this screenshot: This makes me wonder if the block should also allow defining the field name, so we don't lock in to the name |
Oh yes, sorry! |
Thanks for the extra details.
Since this has to be set properly by the site owner first, it may indeed be nice to have matching fields in the block properties. On the plus side, this would also make it easier for non-english folks who may not understand nor want to have a "Signup" field. |
jeffersonrabb, Your synced wpcom patch D35555-code has been updated. |
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. I think the links to Mailchimp are nice to give a bit more info. Since their docs are focussed on creating whole forms from their interface, I wonder if we should link to our own support docs to explain how to do this from the audience settings instead; I struggled a bit to get the fields to work in the first place, so I think some more detailed docs could help there.
Maybe we can ask for help from JPOP Quill and update the existing links once we have docs? What's your take on this?
I also have another comment, but I don't think that's blocking, that can be addressed in a future PR. Approving for now.
jeffersonrabb, Your synced wpcom patch D35555-code has been updated. |
…() rather than indexOf().
jeffersonrabb, Your synced wpcom patch D35555-code has been updated. |
That's a good idea. I'll follow up on this after this lands and we can roll this improvement into future work on the block. |
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 should be good to merge now. 👍
* 8.0 Release: running changelog * Changelog: add #13921 * Changelog: add #13980 * Changelog: add #13905 * Changelog: add #13971 * Changelog: add #13984 * Changelog: add #14009 * Changelog: add #13620 * Remove things that will ship in 7.9.1 * Changelog: add 7.9.1 release (#14044) * Changelog: add base for 7.9.1 release * Update release date and post link * Changelog: add #14066 * Update changelog for 7.9.1 * Changelog: add #13405 * Changelog: add #13841 * Changelog: add #13924 * Changelog: add #13986 * Changelog: add #14010, #14028, #14053, #14055. * Changelog: add #14054 * Changelog: add #14031 * Changelog: add #14039 * Changelog: add #14050 * Changelog: add #14070 * Changelog: add #14082 * Changelog: add #14084 * Changelog: add #14111 * Changelog: add #13961 * Changelog: add #14047 * Changelog: add #14091 * Changelog: add #14108 * Changelog: add #14121
Introduces a few new features to the Mailchimp block:
Preparation
D35074-code
.Client::wpcom_json_api_request_as_blog()
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Discussion here:
p1HpG7-7WS-p2
Testing instructions:
SIGNUP
field following the instructions here. The name of the field must be "SIGNUP"JETPACK__SANDBOX_DOMAIN
is configured for Jetpack development.?amp
to the URL), subscribe. Verify the email appears on the list in the Mailchimp dashboard and that it is in the correct groups and has the correct SIGNUP value.?amp#development=1
to the end of the URL and reload. In the console you should be able to executeAMP.printState()
and get a valid response. Subscribe with a new email address. After success, executeAMP.printState()
again. Verify the state containsmailing_list_status: "subscribed"
andmailing_list_email: {SUBSCRIBED_EMAIL_ADDRESS}
Proposed changelog entry for your changes: