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
Contributor

DavidCramer 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.

Show comment
Hide comment
@westonruter

westonruter Jan 26, 2018

Collaborator

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>
Collaborator

westonruter 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>
Show outdated Hide outdated amp.php Outdated
Show outdated Hide outdated amp.php Outdated
Show outdated Hide outdated amp.php Outdated
Show outdated Hide outdated amp.php Outdated
Show outdated Hide outdated amp.php Outdated
Show outdated Hide outdated includes/class-amp-comment-walker.php Outdated
Show outdated Hide outdated includes/class-amp-comment-walker.php Outdated
Show outdated Hide outdated includes/class-amp-theme-support.php Outdated
Show outdated Hide outdated amp.php Outdated
Show outdated Hide outdated amp.php Outdated
@DavidCramer

This comment has been minimized.

Show comment
Hide comment
@DavidCramer

DavidCramer Jan 27, 2018

Contributor

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

Contributor

DavidCramer commented Jan 27, 2018

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

@DavidCramer

This comment has been minimized.

Show comment
Hide comment
@DavidCramer

DavidCramer Jan 30, 2018

Contributor

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

Contributor

DavidCramer 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

@DavidCramer

This comment has been minimized.

Show comment
Hide comment
@DavidCramer

DavidCramer Jan 31, 2018

Contributor

@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.

Contributor

DavidCramer 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.

@westonruter westonruter changed the title from [WIP] - Add amp-live-list comments to 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