Add AMP support for shortcodes and oEmbeds for Crowdsignal (Polldaddy) by westonruter · Pull Request #11484 · Automattic/jetpack

westonruter · 2019-03-05T19:40:37Z

This consolidates the logic for rendering AMP versions of Crowdsignal/Polldaddy polls and surveys from the AMP plugin. See ampproject/amp-wp#1929 for the code that this PR makes obsolete in the AMP plugin.

This PR also makes sure that a title shortcode attribute gets passed onto the rendered poll link on the frontend.

I assume this code would also need to be copied into the Crowdsignal plugin.

See #9730.

Changes proposed in this Pull Request:

Add first-party AMP compatibility for polls and surveys that are rendered via oEmbeds/shortcodes for Crowdsignal/Polldaddy. Note that the AMP versions of these are just links back to take the poll/survey.

Testing instructions:

To test:

Create a post with the below post_content.
Install the AMP plugin from source.
Check out the remove/bundled-polldaddy branch of the AMP plugin (see Add first-party support for Crowdsignal poll/survey oEmbeds ampproject/amp-wp#1929)
Compare the non-AMP with the AMP version of the post. There should be no AMP validation errors reported by the plugin and each of the polls/surveys should have some representation in the content.

Given this post_content:

<!-- wp:heading -->
<h2>Poll</h2>
<!-- /wp:heading -->

<!-- wp:core-embed/crowdsignal {"url":"https://polldaddy.com/poll/7012505/","type":"rich","providerNameSlug":"crowdsignal","className":""} -->
<figure class="wp-block-embed-crowdsignal wp-block-embed is-type-rich is-provider-crowdsignal"><div class="wp-block-embed__wrapper">
https://polldaddy.com/poll/7012505/
</div><figcaption>Poll oEmbed</figcaption></figure>
<!-- /wp:core-embed/crowdsignal -->

<!-- wp:paragraph -->
<p>And <code>crowdsignal</code> shortcode:</p>
<!-- /wp:paragraph -->

<!-- wp:shortcode -->
[crowdsignal poll=7012505 title="Take this awesome awesome poll!"]
<!-- /wp:shortcode -->

<!-- wp:heading -->
<h2>Survey</h2>
<!-- /wp:heading -->

<!-- wp:core-embed/crowdsignal {"url":"https://rydk.polldaddy.com/s/test-survey","type":"rich","providerNameSlug":"crowdsignal","className":""} -->
<figure class="wp-block-embed-crowdsignal wp-block-embed is-type-rich is-provider-crowdsignal"><div class="wp-block-embed__wrapper">
https://rydk.polldaddy.com/s/test-survey
</div><figcaption>Survey oEmbed</figcaption></figure>
<!-- /wp:core-embed/crowdsignal -->

<!-- wp:paragraph -->
<p>And <code>crowdsignal</code> shortcode:</p>
<!-- /wp:paragraph -->

<!-- wp:shortcode -->
[crowdsignal survey=test-survey title="Take this awesome survey!"]
<!-- /wp:shortcode -->

The non-AMP version looks like this (note the first crowdsignal shortcode poll fails to render for some reason):

And here is the AMP version looks like:

Proposed changelog entry for your changes:

Add AMP support for Crowdsignal/Polldaddy polls and shortcodes.

jetpackbot · 2019-03-05T19:45:26Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against bed4a8d

westonruter · 2019-03-06T00:55:05Z

modules/shortcodes/crowdsignal.php

+ * @param array  $attr  Shortcode attributes.
+ * @return string Embed.
+ */
+function crowdsignal_filter_amp_embed_oembed_html( $cache, $url, $attr ) {


This oEmbed filter should probably be kept in the AMP plugin since Crowdsignal is supported by core directly:

https://github.com/WordPress/wordpress-develop/blob/14cebbe9877c4ef79ea4d79e3d8e1a5581930377/src/wp-includes/class-oembed.php#L65-L67

Implemented now in ampproject/amp-wp#1929

kraftbj

Agree the oEmbed filter should remain in the AMP plugin since this is a core oembed.

kraftbj · 2019-03-11T18:28:26Z

modules/shortcodes/crowdsignal.php

+ * @param array  $attr  Shortcode attributes.
+ * @return string Embed.
+ */
+function crowdsignal_filter_amp_embed_oembed_html( $cache, $url, $attr ) {


kraftbj · 2019-03-11T18:28:55Z

modules/shortcodes/crowdsignal.php

 			$item_id = esc_js( $item_id );

-			if ( $inline ) {
+			if ( Jetpack_AMP_Support::is_amp_request() ) {


Just a note that we'd need to add in support for this function before merging to WP.com.

This would be handled by D22154-code .

…oject/amp-wp#1929

* Initial Changelog for 7.2 * Testing list: add mention of IE11 testing * Initial Changelog for 7.2 * Testing list: add mention of IE11 testing * Add CL for #11224 * Add CL for #11426 * Add CL for #11442 * Add testing instructions for #11224 * Add CL for #11451 * Reclassify CL item * Add testing instructions for #11451 * Add CL for #11486 * Add CL for #11418 * Add CL for #11524 * Add CL and testing instructions for #11449 * Add CL for #11460 * Add CL for #11520 and #11582 * Add CL for #11531 * Add CL #11644 * Add testing instructions for #11644 * Add testing instructions for #11644 * Add CL for #11618 * Uniform changelog lines * CL #11679 * CL #11661 * CL #11654 * CL #11645 * CL #11643 * CL #11636 * CL #11635 and for other PHPCS commits * CL #11627 * CL #11626 * CL #11598 * CL #11596 * Remove nested items for shortcopy. I don't believe the detailed list is helpful * CL #11570 * CL #11569 * CL #11560 * CL #11558 * CL #11555 * CL #6704 * CL #11298 * CL #11324 * CL #11443 * CL #11484 * CL #11516 * CL #11529 * Expand Ads block enhancement CL item

Add AMP support for Crowdsignal shortcodes and oEmbeds

88f7693

westonruter requested a review from a team March 5, 2019 19:40

westonruter mentioned this pull request Mar 5, 2019

Add first-party support for Crowdsignal poll/survey oEmbeds ampproject/amp-wp#1929

Merged

1 task

westonruter mentioned this pull request Mar 5, 2019

AMP compatibility issues #9730

Open

kraftbj added this to the 7.2 milestone Mar 5, 2019

kraftbj added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Shortcodes / Embeds [Status] Needs Review This PR is ready for review. AMP labels Mar 5, 2019

westonruter commented Mar 6, 2019

View reviewed changes

kraftbj requested changes Mar 11, 2019

View reviewed changes

kraftbj 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 Mar 11, 2019

Remove embed_oembed_html filter which remains in AMP plugin via amppr…

bed4a8d

…oject/amp-wp#1929

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! 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 Mar 15, 2019

kraftbj approved these changes Mar 15, 2019

View reviewed changes

kraftbj merged commit 3f9dc9c into Automattic:master Mar 15, 2019

matticbot added the [Status] Needs Changelog label Mar 15, 2019

ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 15, 2019

westonruter deleted the add/crowdsignal-amp-support branch March 15, 2019 20:14

kraftbj added [Status] Has Changelog and removed [Status] Needs Changelog labels Mar 26, 2019

kraftbj added a commit that referenced this pull request Mar 26, 2019

CL #11484

13c1cbc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AMP support for shortcodes and oEmbeds for Crowdsignal (Polldaddy)#11484

Add AMP support for shortcodes and oEmbeds for Crowdsignal (Polldaddy)#11484
kraftbj merged 2 commits intoAutomattic:masterfrom
westonruter:add/crowdsignal-amp-support

westonruter commented Mar 5, 2019 •

edited

Loading

Uh oh!

jetpackbot commented Mar 5, 2019 •

edited

Loading

Uh oh!

westonruter Mar 6, 2019

Uh oh!

kraftbj Mar 11, 2019

Uh oh!

westonruter Mar 15, 2019

Uh oh!

kraftbj left a comment

Uh oh!

kraftbj Mar 11, 2019

Uh oh!

kraftbj Mar 11, 2019

Uh oh!

kraftbj Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

westonruter commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

jetpackbot commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

westonruter Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

kraftbj left a comment

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

westonruter commented Mar 5, 2019 •

edited

Loading

jetpackbot commented Mar 5, 2019 •

edited

Loading