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-live-list comments #909

Merged
merged 27 commits into from Jan 31, 2018

Conversation

Projects
None yet
2 participants
@DavidCramer
Copy link
Contributor

commented Jan 26, 2018

  • Decide on whether to enforce div container instead of ol. Ideal is ol.
  • Figure out how to reset form after submission.
  • Triggering an update does not force a request to update. In short term, just show message saying it will be available soon.
  • Page without comments does not output an amp-live-list to start with.
  • Admin notice on the discussion settings screen to indicate that comments in ascending order is not supported yet.

Fixes #862.
Fixes #797.

@DavidCramer DavidCramer requested a review from westonruter Jan 26, 2018

@westonruter

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

As discussed, to fully support comments in the AMP plugin for WordPress, the amp-live-list component really needs to support the sort order, but it is not yet: ampproject/amphtml#5396

The ordering of WP comments is configurable in the admin:

image

For the immediate term we'll be forcing the option to be “newer” and display a notice:

image

So when amp theme support is present, we'll need to add a filter for option_comment_order to have it return 'desc'. And then some JS will need to be injected onto the Discussion admin screen to disable the select#comment_order element and then to inject the following HTML at the end of its containing fieldset:

<div class="notice notice-info inline"><p><?php echo wp_kses_post( __( 'Note: AMP does not yet <a href="https://github.com/ampproject/amphtml/issues/5396" target="_blank">support ascending</a> comments with newer entries appearing at the bottom.', 'amp' ) ); ?></p></div>
amp.php Outdated
* @param string $message The error message to send.
*/
function amp_send_error_json( $message ) {
header( 'HTTP/1.1 400 BAD REQUEST' );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

You can use status_header( 400 ) instead.

amp.php Outdated
*/
function amp_send_error_json( $message ) {
header( 'HTTP/1.1 400 BAD REQUEST' );
wp_send_json( array( 'error' => strip_tags( $message, 'strong' ) ) );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

No need to strip_tags() here, as amp-mustache will escape properly and allow formatting as may be desired.

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Jan 30, 2018

Author Contributor

@westonruter I used striptags because amp-mustache takes the text from element so it doesn't go deeper. meaning that anything within child elements are totally removed. So <p><strong>ERROR</strong>:Comment is empty.</p> becomes :Comment is empty
I think we could get all the basic formatting tags in a striptags. Which should work.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 30, 2018

Member

Great to know. Thanks for sharing.

amp.php Outdated
*/
function amp_send_error_json( $message ) {
header( 'HTTP/1.1 400 BAD REQUEST' );
wp_send_json( array( 'error' => strip_tags( $message, 'strong' ) ) );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Note that a WP_Error instance can be passed to wp_die(), so that needs to be accounted for.

amp.php Outdated
// Add amp comment hooks.
add_filter( 'comment_post_redirect', 'amp_comment_post_handler', PHP_INT_MAX, 2 );
add_filter( 'wp_die_handler', 'amp_set_comment_submit_die_handler' );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Now that PHP 5.3 is the minimum requirement, we can use closures here to prevent the proliferation of one-liner functions. So this can be:

add_filter( 'wp_die_handler', function() {
    return function( $error ) {
        status_header( 400 );
        if ( is_wp_error( $error ) ) {
            $error = $error->get_error_message();
        }
        wp_send_json( compact( 'error' ) );
    };
} );

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Jan 27, 2018

Author Contributor

awesome! I forgot that the min version was increased. great news!

amp.php Outdated
function amp_hook_comments_maybe() {
$method = filter_input( INPUT_SERVER, 'REQUEST_METHOD', FILTER_SANITIZE_STRING );
$is_amp_submit = filter_input( INPUT_GET, '__amp_source_origin', FILTER_VALIDATE_URL );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Let's opt to use $_SERVER and $_GET to facilitate unit testing.

$args = array_slice( func_get_args(), 4 );
$output = '<amp-live-list layout="container" data-poll-interval="15000" data-max-items-per-page="20" id="amp-live-comments-list">';

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

The id should probably be unique. It's possible that more than one comment loop could be displayed on a page. So there should perhaps be a new static member variable that increments as the method is called.

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Jan 27, 2018

Author Contributor

Good point! will address it.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 27, 2018

Member

Actually, probably should use the post ID in the id here, as otherwise there could be problems with pagination.

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Jan 27, 2018

Author Contributor

Was thinking the same thing. Will experiment and on it.

* @return string XHTML of the specified page of elements.
*/
public function paged_walk( $elements, $max_depth, $page_num, $per_page ) {
if ( empty( $elements ) || $max_depth < - 1 ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Trivial: Remove space between - and 1

$amp_walker = new AMP_Comment_Walker();
$args['walker'] = $amp_walker;
// @todo Make threaded work by setting 'data-update-time' of the parent first level parent with the last timestamp.
$args['max_depth'] = 1;

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Yeah, we need to be able to remove this constraint.

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Jan 27, 2018

Author Contributor

Ye, this is temporary, I figured out how to do it. will experiment and make it work!

amp.php Outdated
/**
* Hook into a comment submission if an AMP xhr post request.
*/
function amp_hook_comments_maybe() {

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Rename this to amp_handle_comment_post.

amp.php Outdated
function amp_hook_comments_maybe() {
$method = filter_input( INPUT_SERVER, 'REQUEST_METHOD', FILTER_SANITIZE_STRING );
$is_amp_submit = filter_input( INPUT_GET, '__amp_source_origin', FILTER_VALIDATE_URL );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

The variable name here implies it is a boolean. Let's change it to $amp_source_origin

@DavidCramer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2018

@westonruter Thanks for the thorough review. Good suggestions made. Will work on these and update promptly. Thanks!

}
} );

})( jQuery );

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Jan 30, 2018

Author Contributor

@westonruter I think this could easily have been a simple inline script alongside the notice, but I made it a asset file for now, just in case.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 30, 2018

Member

Thanks. I've decided to go ahead and inline it. See 310a74a

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Jan 31, 2018

Author Contributor

Perfect. thought that would be the case. thanks for sorting it for me.

@DavidCramer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

@westonruter You can review this again now I think I have addressed all issues as well as made changes necessary for xwp/ampnews#52

@westonruter westonruter force-pushed the add/amp-live-list-comments branch from 9e7e6ea to 8f1b37c Jan 30, 2018

@DavidCramer

This comment has been minimized.

Copy link
Contributor

commented on includes/class-amp-comment-walker.php in 91b0c8d Jan 31, 2018

@westonruter I always thought strtotime acted as a sanitisation since it's an int. But I see that it's always good to esc_attr regardless. thanks for pointing it out.

* @param string $text Cancel comment reply link text.
* @return string Cancel reply link.
*/
public function filter_cancel_comment_reply_link( $formatted_link, $link, $text ) {

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Jan 31, 2018

Author Contributor

@westonruter should be a static function.

@westonruter westonruter changed the title [WIP] - Add amp-live-list comments Add amp-live-list comments Jan 31, 2018

westonruter added some commits Jan 31, 2018

Fix setting initial replyToName state
Also fix php notices regarding undefined pagenow var and pass var by ref
Move function with closures out of amp.php for sake of PHP 5.2 check
Remove unused amp_comments_order_disable_scripts() function.

@westonruter westonruter added this to the v0.7 milestone Jan 31, 2018

@westonruter westonruter merged commit ae997c4 into develop Jan 31, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/amp-live-list-comments branch Jan 31, 2018

kienstra added a commit that referenced this pull request Mar 29, 2018

Add test class for AMP_Comment_Walker.
And test all of its methods.
@todo: tests for remaining methods in PR #909.

kienstra added a commit that referenced this pull request Mar 30, 2018

Add test class for AMP_Comment_Walker.
And test all of its methods.
@todo: tests for remaining methods in PR #909.

westonruter added a commit that referenced this pull request Apr 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.