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

Let amp-live-list for comments be opt-in #1028

Closed
westonruter opened this issue Mar 20, 2018 · 9 comments
Closed

Let amp-live-list for comments be opt-in #1028

westonruter opened this issue Mar 20, 2018 · 9 comments
Assignees
Projects
Milestone

Comments

@westonruter
Copy link
Member

We should add a theme support flag for comments_live_list to control whether or not comment submission is intercepted. Without flag, default full-page-reload behavior should be retained to allow themes without amp-live-list in comments to work normally.

The theme can do:

add_theme_support( 'amp', array(
	'comments_live_list' => true,
) );

And then the plugin can read it like this:

<?php
add_filter( 'comment_post_redirect', function( $url, $comment ) {
	$theme_support = get_theme_support( 'amp' );

	// Return with simply success when amp-live-list will show newly-posted comment.
	if ( ! empty( $theme_support['comments_live_list'] ) ) {
		wp_send_json_success();
	}

	// Add the comment ID to the URL to force AMP to refresh the page.
	$url = add_query_arg( 'comment', $comment->comment_ID, $url );

	return $url;
}, PHP_INT_MAX, 2 );

This will allow amp support to be added to an existing theme without requiring its comments template to be retrofitted with amp-live-list.

Alternatively, if we think that amp-live-list should really be the default we want to always be used, then there could instead be an opt-out flag.

@westonruter westonruter added this to the v0.7 milestone Mar 20, 2018
@westonruter
Copy link
Member Author

Naturally the wp_send_json_success() bit would change in light of #1020 (comment)

@DavidCramer
Copy link
Contributor

Thanks @westonruter .

@westonruter westonruter added this to Definition in v0.7 Mar 20, 2018
@DavidCramer DavidCramer moved this from Definition to Ready for Review in v0.7 Mar 20, 2018
@westonruter westonruter moved this from Ready for Review to QA in v0.7 Mar 21, 2018
@westonruter westonruter reopened this Mar 21, 2018
@westonruter
Copy link
Member Author

westonruter commented Mar 21, 2018

To test without opt-in, making a comment at: https://paired-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/amp/

Should result in a page refresh with a URL like https://paired-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/amp/?comment=34#comment-34

For a native AMP site where the flag is present, there should be no refresh and instead the Update Comments button should appear to update the live list: https://dev-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/

@westonruter
Copy link
Member Author

Three issues I see:

  1. When in paired mode, if I submit the comment at https://paired-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/amp/ then it will redirect me to the non-AMP article. The comment_post_redirect filter should be making sure that if paired mode is enabled that it redirects to the AMP version of the post instead.

  2. Related: when in non-paired mode such as at https://paired-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/#respond I cannot submit the form. Why? Because the amp-runtime script is erroneously getting enqueued. This results in:

image

  1. When submitting a comment from a site I can see now that _wp_amp_action_xhr_converted really should be used to determine whether the logic in AMP_Theme_Support::handle_xhr_request() runs.

@westonruter westonruter reopened this Mar 21, 2018
@westonruter westonruter moved this from QA to In Progress in v0.7 Mar 21, 2018
@westonruter westonruter moved this from In Progress to Ready for Review in v0.7 Mar 22, 2018
@westonruter westonruter moved this from Ready for Review to QA in v0.7 Mar 22, 2018
@westonruter
Copy link
Member Author

To test, try commenting on AMP article in paired mode: https://paired-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/amp/

And try commenting on the non-AMP version of the article: https://paired-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/

In both cases, the page should reload after submitting with the comment scrolled into view.

Also try commenting on AMP article in native mode: https://dev-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/

@kienstra
Copy link
Contributor

Request For Testing

Hi @csossi,
Are you available to test this, using @westonruter's instructions above?

@csossi
Copy link

csossi commented Mar 28, 2018

yes, will be on this today/tomorrow @kienstra @westonruter

@csossi
Copy link

csossi commented Mar 29, 2018

  • when I post a comment on native mode site, the display refreshes with the "awaiting moderation" message appearing just below the comment form - doesn't reposition anywhere. Can I assume that's the intent here, @westonruter ?

@csossi csossi moved this from QA to Ready for Merging in v0.7 Mar 29, 2018
@westonruter
Copy link
Member Author

@csossi After submitting the comment, it should look like this with the New Comment(s) button momentarily appearing (within 15 seconds) above the form:

image

Upon clicking the New Comments button it should scroll you up to your posted comment, and look like this:

image

@kevincoleman kevincoleman moved this from Ready for Merging to Production Release in v0.7 May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v0.7
Production Release
Development

No branches or pull requests

5 participants