Enable related posts for AMP #13028

gravityrail · 2019-07-11T05:05:08Z

Currently the related posts block is disabled for all AMP requests because they don't support client-side JS. However, the Gutenberg block does server side rendering. This patch reuses the Gutenberg code in kind of a brute-force way to enable Related Posts in AMP.

cc @westonruter @jeffersonrabb

Changes proposed in this Pull Request:

Enable Related Posts for AMP pages

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

This adds Related Posts support to AMP pages

Testing instructions:

Install and activate the AMP plugin (tested with v1.2)
Enable Related posts
View the AMP version of a page by appending ?amp
Related posts should be displayed
Try changing some settings (e.g. enable/disable thumbnails, enable/disable heading). Should work similarly to front-end

This is the most minimal, brute-force version I could create. It may create invalid AMP in other ways (e.g. too much CSS). Comments and corrections welcome.

Proposed changelog entry for your changes:

Enable Related Posts for AMP pages

matticbot · 2019-07-11T05:05:14Z

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

jetpackbot · 2019-07-11T05:06:52Z

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: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against 6725d38

matticbot · 2019-07-11T05:12:28Z

gravityrail, Your synced wpcom patch D30356-code has been updated.

matticbot · 2019-07-11T05:13:44Z

gravityrail, Your synced wpcom patch D30356-code has been updated.

modules/related-posts/jetpack-related-posts.php

westonruter · 2019-07-11T05:27:15Z

I enabled:

Rendering as:

When thumbnails are hidden:

With heading hidden (with social sharing icons included above to show spacing):

Here's the https://dev-westonruter.pantheonsite.io/2019/03/21/using-the-amp-plugin-to-protect-site-visitors-and-debug-security-vulnerabilities/

It looks good, except for the thumbnails being a bit off.

westonruter · 2019-07-11T05:33:41Z

In Reader mode there is no styling, so either the styling needs to be added or the Related Posts should be omitted if ! current_theme_supports( 'amp' ):

westonruter · 2019-07-11T05:35:55Z

It looks good, except for the thumbnails being a bit off.

It's not the thumbnail, it's the text link under the thumbnail, where there is a styling problem:

matticbot · 2019-07-11T09:33:23Z

gravityrail, Your synced wpcom patch D30356-code has been updated.

jeffersonrabb

Looks good - only change needed is the $enqueue_script logic.

modules/related-posts/jetpack-related-posts.php

jeffersonrabb · 2019-07-26T10:41:14Z

modules/related-posts/jetpack-related-posts.php

@@ -235,7 +258,7 @@ public function get_target_html() {
 	 *
 	 * @returns string
 	 */
-	public function get_target_html_unsupported() {
+	public function get_client_rendered_html_unsupported() {


This is probably out of scope of this PR, but is Related Posts supposed to appear in the loop? The doc comment implies not, but I'm seeing them on a test homepage (both on this branch and master).

Co-Authored-By: Jefferson Rabb <jefferson.rabb@automattic.com>

matticbot · 2019-07-28T18:30:46Z

gravityrail, Your synced wpcom patch D30356-code has been updated.

jeffersonrabb

Looks good.

dereksmart · 2019-07-29T13:48:07Z

Added to 7.6 milestone as per request of @gravityrail

matticbot · 2019-07-29T21:10:54Z

gravityrail, Your synced wpcom patch D30356-code has been updated.

jeherve

This works for me, although the whole section looks pretty bad without any CSS. I don't know if we should ship this change now, without applying basic styles to the related posts.

modules/related-posts/jetpack-related-posts.php

Enable related posts for AMP

08c27d9

gravityrail requested a review from a team July 11, 2019 05:05

matticbot added the Touches WP.com Files label Jul 11, 2019

gravityrail added AMP [Feature] Related Posts [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jul 11, 2019

gravityrail mentioned this pull request Jul 11, 2019

Port Related Posts to work with AMP #9556

Closed

gravityrail added this to the 7.6 milestone Jul 11, 2019

Don't enqueue JS for AMP

7693ce9

Cautious extra check

c1b455a

gravityrail requested a review from jeffersonrabb July 11, 2019 05:19

westonruter reviewed Jul 11, 2019

View reviewed changes

modules/related-posts/jetpack-related-posts.php Outdated Show resolved Hide resolved

modules/related-posts/jetpack-related-posts.php Show resolved Hide resolved

Improve names and comments

293a756

jeherve assigned gravityrail Jul 12, 2019

jeherve removed this from the 7.6 milestone Jul 25, 2019

jeffersonrabb suggested changes Jul 26, 2019

View reviewed changes

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jul 26, 2019

Update modules/related-posts/jetpack-related-posts.php

04d2677

Co-Authored-By: Jefferson Rabb <jefferson.rabb@automattic.com>

jeffersonrabb previously approved these changes Jul 29, 2019

View reviewed changes

dereksmart removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jul 29, 2019

dereksmart added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Jul 29, 2019

dereksmart added this to the 7.6 milestone Jul 29, 2019

Related Posts: PHPCS and Jetpack Codex-related changes

6725d38

jeherve dismissed jeffersonrabb’s stale review via 6725d38 July 29, 2019 21:10

jeherve approved these changes Jul 29, 2019

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jul 29, 2019

zinigor merged commit 91c5676 into master Jul 29, 2019

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 29, 2019

zinigor deleted the try/amp-related-posts branch July 29, 2019 21:53

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Jul 30, 2019

westonruter mentioned this pull request Jan 17, 2020

Use Jetpack_RelatedPosts::get_options() to get options for server-rendered Related Posts #14386

Merged

westonruter reviewed Jan 17, 2020

View reviewed changes

modules/related-posts/jetpack-related-posts.php Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable related posts for AMP #13028

Enable related posts for AMP #13028

gravityrail commented Jul 11, 2019 •

edited

Loading

matticbot commented Jul 11, 2019

jetpackbot commented Jul 11, 2019 •

edited

Loading

matticbot commented Jul 11, 2019

matticbot commented Jul 11, 2019

westonruter commented Jul 11, 2019 •

edited

Loading

westonruter commented Jul 11, 2019

westonruter commented Jul 11, 2019

matticbot commented Jul 11, 2019

jeffersonrabb left a comment

jeffersonrabb Jul 26, 2019

matticbot commented Jul 28, 2019

jeffersonrabb left a comment

dereksmart commented Jul 29, 2019

matticbot commented Jul 29, 2019

jeherve left a comment •

edited

Loading

Enable related posts for AMP #13028

Enable related posts for AMP #13028

Conversation

gravityrail commented Jul 11, 2019 • edited Loading

Changes proposed in this Pull Request:

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

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Jul 11, 2019

jetpackbot commented Jul 11, 2019 • edited Loading

matticbot commented Jul 11, 2019

matticbot commented Jul 11, 2019

westonruter commented Jul 11, 2019 • edited Loading

westonruter commented Jul 11, 2019

westonruter commented Jul 11, 2019

matticbot commented Jul 11, 2019

jeffersonrabb left a comment

Choose a reason for hiding this comment

jeffersonrabb Jul 26, 2019

Choose a reason for hiding this comment

matticbot commented Jul 28, 2019

jeffersonrabb left a comment

Choose a reason for hiding this comment

dereksmart commented Jul 29, 2019

matticbot commented Jul 29, 2019

jeherve left a comment • edited Loading

Choose a reason for hiding this comment

gravityrail commented Jul 11, 2019 •

edited

Loading

jetpackbot commented Jul 11, 2019 •

edited

Loading

westonruter commented Jul 11, 2019 •

edited

Loading

jeherve left a comment •

edited

Loading