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 support for comments #797

Closed
westonruter opened this Issue Nov 6, 2017 · 8 comments

Comments

Projects
5 participants
@westonruter
Copy link
Member

westonruter commented Nov 6, 2017

In order to make AMP Canonical possible (#668) we'll need native support for WordPress commenting. We could create an amp-wordpress-comments component, and extend comment_form() and wp_list_comments() to accept format=amp which would then render this.

See also:
https://www.ampproject.org/docs/tutorials/login_requiring
https://ampbyexample.com/samples_templates/comment_section/

@amedina

This comment has been minimized.

Copy link
Member

amedina commented Nov 6, 2017

@westonruter creating an amp-wordpress-comments component may not be necessary (and it would be hard to land it into the library); but the referenced methods can be extended to generate a AMP-compatible format.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Nov 6, 2017

I saw the amp-facebook-comments component so I thought that might be a path, but yeah, it probably wouldn't be necessary since we could just use amp-form and other existing components.

@amedina

This comment has been minimized.

Copy link
Member

amedina commented Nov 6, 2017

Thinking more in general, I wonder if it would make sense to propose the creation of a set of AMP components that are specific to WordPress; given the prevalence of WP in the web ecosystem, the requirement of "general utility" would be satisfied. Will run the idea around to gather perspective.

@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented Jan 16, 2018

further information on this is on #862

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 16, 2018

The bare minimum it seems to add AMP support for the comment list is just to make sure that Gravatars use amp-img as opposed to img. In short, the following could be adapted and integrated into the theme support class:

add_filter( 'get_avatar', function( $avatar ) {
	$avatar = str_replace( '<img', '<amp-img', $avatar );
	return $avatar;
} );
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 16, 2018

It would be great to eventually support amp-list for comments, but that will require a lot more work, especially to support nested comment replies.

@alcurrie

This comment has been minimized.

Copy link
Collaborator

alcurrie commented Feb 6, 2018

Functionality looks good and works well, but I noticed a couple of items that were not as expected:

  1. Issue: Comment moderation does not display comments for previously approved comment authors.
    Expected
    In the Discussion Settings, when the Before a comment appears: Comment must be manually approved (is 'unchecked'), and Comment Author must have a previously approved comment is checked (See screen cast of settings https://cl.ly/442k1O2z1J0b)
    I expect that when I enter the same Name and email as a user whose comments have already been approved, that my comment will display right away.
    Actual
    Even if the comment author has been previously approved, comment does not display.

  2. The confirmation of the comment post, is too close to the Post comment button:
    screen shot 2018-02-05 at 7 27 41 pm

  3. From a usability perspective the comments seem quite far away from the actual article with several other articles between the bottom of the article the user is commenting on, with 3 other articles between the article and the comments on the article
    https://cl.ly/1x0g0H0K421k

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 7, 2018

From a usability perspective the comments seem quite far away from the actual article with several other articles between the bottom of the article the user is commenting on, with 3 other articles between the article and the comments on the article
https://cl.ly/1x0g0H0K421k

This is a known issue which has to be fixed in AMP itself. See ampproject/amphtml#5396 (comment)

We may want to show the comment form above the list because of this?

@ThierryA ThierryA added the Sprint 4 label Mar 8, 2018

@ThierryA ThierryA added this to QA in v0.7 Mar 8, 2018

@ThierryA ThierryA moved this from QA to Beta Release in v0.7 Mar 8, 2018

@kevincoleman kevincoleman moved this from Beta Release 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