Skip to content
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

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

Merged
merged 2 commits into from
Mar 15, 2019

Conversation

westonruter
Copy link
Contributor

@westonruter westonruter commented Mar 5, 2019

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:

  1. Create a post with the below post_content.
  2. Install the AMP plugin from source.
  3. 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)
  4. 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):

screen shot 2019-03-05 at 11 34 32

And here is the AMP version looks like:

screen shot 2019-03-05 at 11 34 50

Proposed changelog entry for your changes:

  • Add AMP support for Crowdsignal/Polldaddy polls and shortcodes.

@jetpackbot
Copy link
Collaborator

jetpackbot commented Mar 5, 2019

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

@kraftbj kraftbj added this to the 7.2 milestone Mar 5, 2019
@kraftbj kraftbj added [Type] 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
* @param array $attr Shortcode attributes.
* @return string Embed.
*/
function crowdsignal_filter_amp_embed_oembed_html( $cache, $url, $attr ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented now in ampproject/amp-wp#1929

Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Agreed.

@@ -209,7 +235,9 @@ function crowdsignal_shortcode( $atts ) {

$item_id = esc_js( $item_id );

if ( $inline ) {
if ( Jetpack_AMP_Support::is_amp_request() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be handled by D22154-code .

@kraftbj 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
@kraftbj 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 kraftbj merged commit 3f9dc9c into Automattic:master Mar 15, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 15, 2019
@westonruter westonruter deleted the add/crowdsignal-amp-support branch March 15, 2019 20:14
kraftbj added a commit that referenced this pull request Mar 26, 2019
kraftbj added a commit that referenced this pull request Mar 27, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP [Feature] Shortcodes / Embeds [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants