Skip to content

Mailchimp Block: AMP Support#12695

Merged
jeffersonrabb merged 4 commits intomasterfrom
add/mailchimp-block-amp-support
Oct 3, 2019
Merged

Mailchimp Block: AMP Support#12695
jeffersonrabb merged 4 commits intomasterfrom
add/mailchimp-block-amp-support

Conversation

@jeffersonrabb
Copy link
Copy Markdown
Contributor

@jeffersonrabb jeffersonrabb commented Jun 14, 2019 •
edited by jeherve
Loading

This PR introduces AMP support for the Mailchimp block. The functionality of the block should be identical in AMP and non-AMP pages.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Discussion of Jetpack AMP compatibility here: p1HpG7-6nB-p2

Testing instructions:

  • Apply D29462-code to sandbox.
  • Install AMP plugin (https://wordpress.org/plugins/amp/) on local testing instance, enable AMP, set to Transitional mode.
  • Add Mailchimp block to a post (you'll need to link to a Mailchimp account to test).
  • Publish the post and view. In non-AMP mode, try adding various emails, and verify that they are added to your Mailchimp list.
  • Switch the page to AMP mode, and try the same. The results should be exactly the same.

@jeffersonrabb jeffersonrabb requested a review from a team June 14, 2019 12:19
@jeffersonrabb jeffersonrabb self-assigned this Jun 14, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Jun 14, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello jeffersonrabb! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D29459-code before merging this PR. Thank you!

@jeffersonrabb jeffersonrabb added the DO NOT MERGE don't merge it! label Jun 14, 2019
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Jun 14, 2019 •
edited
Loading

Warnings
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against a8f8135

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Jun 14, 2019

jeffersonrabb, Your synced wpcom patch D29459-code has been updated.

@jeffersonrabb jeffersonrabb added [Status] Needs Review This PR is ready for review. [Block] Mailchimp and removed DO NOT MERGE don't merge it! labels Jun 14, 2019
@jeherve jeherve added AMP [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Jun 17, 2019
@jeherve jeherve added this to the 7.5 milestone Jun 17, 2019
@jeffersonrabb jeffersonrabb added AMP and removed AMP labels Jun 18, 2019
@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Jun 19, 2019

@jeffersonrabb can you please commandeer this wpcom patch and have someone from your team review? D29459-code

thanks!

brbrr
brbrr reviewed Jun 20, 2019
View reviewed changes
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jun 20, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Jun 20, 2019

jeffersonrabb, Your synced wpcom patch D29459-code has been updated.

@jeherve jeherve force-pushed the add/mailchimp-block-amp-support branch from ed5fb5a to 6687091 Compare June 20, 2019 20:44
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Jun 20, 2019

jeffersonrabb, Your synced wpcom patch D29459-code has been updated.

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jun 20, 2019
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jun 20, 2019

Rebasing to fix tests.

jeherve
jeherve requested changes Jun 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It seems to work well, but I still get an AMP validation error:

image

@jeherve jeherve removed the [Status] Needs Review This PR is ready for review. label Jun 21, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Aug 27, 2019

jeffersonrabb, Your synced wpcom patch D29459-code has been updated.

@jeffersonrabb
Copy link
Copy Markdown
Contributor Author

jeffersonrabb commented Sep 5, 2019

Hold off on further review pending resolution of a couple of issues in D29462-code.

D29462 has been revised, and this work should now be ready for a new look.

cc: @georgeh

@jeffersonrabb jeffersonrabb added [Status] Needs Review This PR is ready for review. and removed [Status] Blocked / Hold labels Sep 5, 2019
@jeherve jeherve added this to the 7.8 milestone Sep 6, 2019
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Sep 24, 2019

I commented on the Phab diff, as I still experience some issues when trying to submit email addresses that are already subscribed to the site.

@jeherve jeherve modified the milestones: 7.8, 7.9 Sep 24, 2019
claudiulodro
claudiulodro previously approved these changes Oct 1, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@claudiulodro claudiulodro left a comment

Choose a reason for hiding this comment

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

LGTM

@jeherve jeherve force-pushed the add/mailchimp-block-amp-support branch from 945ad31 to a8f8135 Compare October 2, 2019 15:11
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Oct 2, 2019

jeffersonrabb, Your synced wpcom patch D29459-code has been updated.

jeherve
jeherve approved these changes Oct 2, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work well in my tests. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 2, 2019
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Oct 2, 2019

Noting that I had to rebase, hence why the diff was updated.

@jeffersonrabb jeffersonrabb merged commit b657c4e into master Oct 3, 2019
@jeffersonrabb jeffersonrabb deleted the add/mailchimp-block-amp-support branch October 3, 2019 12:12
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 3, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #12695
b1fa84b
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
@jeherve jeherve mentioned this pull request Nov 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

@claudiulodro claudiulodro claudiulodro left review comments

@artpi artpi Awaiting requested review from artpi

@kraftbj kraftbj Awaiting requested review from kraftbj

@georgeh georgeh Awaiting requested review from georgeh

+2 more reviewers

@westonruter westonruter westonruter left review comments

@brbrr brbrr brbrr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jeffersonrabb jeffersonrabb

Labels

AMP [Block] Mailchimp [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files

Projects

None yet

Milestone

7.9

Development

Successfully merging this pull request may close these issues.

8 participants

@jeffersonrabb @matticbot @jetpackbot @dereksmart @jeherve @westonruter @brbrr @claudiulodro