Skip to content

Introduce Mailchimp Shortcode#10597

Merged
jeherve merged 43 commits intomasterfrom
mailchimp/introduce-shortcode
Jan 7, 2019
Merged

Introduce Mailchimp Shortcode#10597
jeherve merged 43 commits intomasterfrom
mailchimp/introduce-shortcode

Conversation

@artpi
Copy link
Copy Markdown
Contributor

@artpi artpi commented Nov 12, 2018

This PR introduces 2 things:

  1. Mailchimp shortcode
  2. Mailchimp gutenberg block.

More details in the master thread: p5uIfZ-8BV-p2

The code behind this is already merged for WPCOM simple. If you want to test the UX - any simple site will work for proxied a12s

How this works

This enables visitors to sign up for a MailChimp list.

User needs to establish a Mailchimp connection in sharing section in calypso. There, they can use oauth flow to establish the connection and then select a list where subscribers will be saved.
( We imagine that this feature may be gated behing a plan - more info in the master thread )

Shortcode

Once connection is established, shortcode [jetpack-email-subscribe] will display this signup form.

screen shot 2018-11-12 at 18 14 01 pm

Once visitor signs up, her email is saved to a previously selected emaiil list in Mailchimp. We don't use it ourselves.
All email comes from the site owner through mailchimp

Block

Once connection is established, site owner can use jetpack/gutenberg block.
If connection is NOT established, they get a prompt with the link to go to /sharing.

Screenshots

Block

Block, connection not set up

zrzut ekranu 2018-12-28 o 15 30 05

Testing instructions

  1. Check out this PR on your Jetpack
  2. Build blocks from calypso, the UI code is already merged
  3. You need to log in as an Automattician
  4. In /sharing you need to set up mailchimp connection and choose a list:
    screen shot 2018-11-12 at 18 13 20 pm
  5. Drop a [jetpack-email-subscribe] shortcode anywhere to test shortcode
  6. Use mailchimp block for testing block.
  7. Once you subscribe, your email will end up in the list set up in point 2.

@artpi artpi added [Status] Needs Review This PR is ready for review. New Feature labels Nov 12, 2018
@artpi artpi self-assigned this Nov 12, 2018
@artpi artpi requested a review from a team November 12, 2018 17:22
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Nov 12, 2018

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

This is 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 00de3a7

Comment thread modules/email-subscribe/email-subscribe.php Outdated
@jeherve jeherve added [Feature] Shortcodes / Embeds [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 Nov 13, 2018
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.

Since you are adding a new shortcode, I would recommmend that you place it with the other shortcodes in Jetpack, as part of the Shortcodes module in modules/shortcodes.

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.

A few more remarks about things that could be improved.

Comment thread modules/email-subscribe/email-subscribe.php Outdated
Comment thread modules/email-subscribe/email-subscribe.php Outdated
Comment thread modules/email-subscribe/email-subscribe.php Outdated
@artpi artpi 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 Nov 13, 2018
@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 Nov 13, 2018
@artpi artpi 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 Nov 13, 2018
@artpi
Copy link
Copy Markdown
Contributor Author

artpi commented Nov 13, 2018

@jeherve I implemented all changes here :) If anything else needs improvement - please let me know :)

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.

A few more changes I would recommend.

Comment thread modules/shortcodes/email-subscribe.php
Comment thread modules/shortcodes/email-subscribe.php Outdated
Comment thread modules/shortcodes/js/jetpack-email-subscribe.js Outdated
@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 Nov 13, 2018
@artpi artpi added the [Status] Needs Review This PR is ready for review. label Nov 13, 2018
@artpi
Copy link
Copy Markdown
Contributor Author

artpi commented Jan 4, 2019

If we don't have a better solution for this, can this be documented in code comments? The combination of prevent_jetpack_register_block(), register_gutenberg_block(), and jetpack_register_block() is pretty confusing.

Thank you for catching that!
I swear, there was a comment that got lost in the rebase. Added one now.

@artpi
Copy link
Copy Markdown
Contributor Author

artpi commented Jan 4, 2019

I implemented following fixes from @jeffersonrabb comments:

  1. After a failed email submission, the error message is generic and recommends a page refresh. These messages should indicate the reason for failure. I imagine the most common cause of failure will be an email that's already on the list, and if the user isn't informed of this they'll try over and over until they get frustrated.

Yes, this is true. When I think about it its not actually the error - end state should be the same as success. User is on the list.
I resolve this by showing success message.

  1. This one could probably go either way, but there seems to be a Gutenberg convention of not building titles in blocks.

good point. I made title an optional parameter and don't include a default one, so its not showing.
However, for quick copy-pasting of shortcodes and quicker block setup, you can set it.

  1. If there are multiple instances of the block (or shortcode) on a page, clicking Submit in one appears to submit them all. The submitted button state is applied to all, and all get the same success/failure notification.

Yes. I fixed that.

@artpi
Copy link
Copy Markdown
Contributor Author

artpi commented Jan 4, 2019

I have included recent changes in this PR to D22833-code .
On monday, Ill merge it and rebase D22705-code

I dont want to merge D22705-code until we are sure that we want this fix for SSR to be merged.

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 looks good to me now. 👍

*
* @return array
*/
public function prevent_jetpack_register_block( $blocks ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We are doing something wrong here. We should always register the block using jetpack_register_block
If you have specific visibility requirement that depend blog info (options, plan etc) You will need to use the callback. See https://github.com/Automattic/jetpack/blob/master/modules/simple-payments/simple-payments.php#L61 as an example.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this was discussed here:
#10597 (comment)

@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 Jan 7, 2019
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.

Works well for me now. Merging.

@jeherve jeherve merged commit 1d4dffb into master Jan 7, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 7, 2019
@jeherve jeherve deleted the mailchimp/introduce-shortcode branch January 7, 2019 14:45
jeherve pushed a commit that referenced this pull request Jan 7, 2019
This PR introduces 2 things:
1. Mailchimp shortcode
2. Mailchimp gutenberg block.

More details in the master thread: p5uIfZ-8BV-p2

**The code behind this is already merged for WPCOM simple. If you want to test the UX - any simple site will work for proxied a12s**

## How this works

This enables visitors to sign up for a MailChimp list.

User needs to establish a Mailchimp connection in sharing section in calypso. There, they can use oauth flow to establish the connection and then select a list where subscribers will be saved.
( We imagine that this feature may be gated behing a plan - more info in the master thread )



### Shortcode

Once connection is established, shortcode `[jetpack-email-subscribe]` will display this signup form.

![screen shot 2018-11-12 at 18 14 01 pm](https://user-images.githubusercontent.com/3775068/48363970-76b06a00-e6a7-11e8-867b-e2be6927b1bf.png)

Once visitor signs up, her email is saved to a previously selected emaiil list in Mailchimp. We don't use it ourselves.
All email comes from the site owner through mailchimp

### Block

Once connection is established, site owner can use `jetpack/gutenberg` block.
If connection is NOT established, they get a prompt with the link to go to /sharing.


## Screenshots

### Block

![](https://user-images.githubusercontent.com/3775068/48910903-78c9b400-ee71-11e8-8a9d-ac85d5b11c87.png)

### Block, connection not set up


![zrzut ekranu 2018-12-28 o 15 30 05](https://user-images.githubusercontent.com/3775068/50518375-9e239080-0ab5-11e9-8639-ff887776c332.png)


## Testing instructions

1. Check out this PR on your Jetpack
2. Build blocks from calypso, the UI code is already merged
1. You need to log in as an Automattician
2. In /sharing you need to set up mailchimp connection and choose a list:
![screen shot 2018-11-12 at 18 13 20 pm](https://user-images.githubusercontent.com/3775068/48364106-d9096a80-e6a7-11e8-8513-5ea9279fd7e9.png)
3. Drop a `[jetpack-email-subscribe]` shortcode anywhere to test shortcode
4. Use `mailchimp` block for testing block.
5. Once you subscribe, your email will end up in the list set up in point 2.
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jan 7, 2019

Cherry-picked to branch-6.9 in 2e30de4

@jeffersonrabb
Copy link
Copy Markdown
Contributor

Proposed change to the block here: Automattic/wp-calypso#29929. Removes Server-Side Rendering, could simplify some of the registration questions.

@ockham ockham mentioned this pull request Jan 7, 2019
6 tasks
ockham added a commit that referenced this pull request Jan 8, 2019
@jeherve jeherve mentioned this pull request Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Shortcodes / Embeds [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack New Feature [Pri] High Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants