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

Prevent failed attempts to override the reply-title heading in comment form #4388

Merged
merged 1 commit into from
Mar 17, 2020

Conversation

westonruter
Copy link
Member

Summary

This is a short-term fix to prevent attempting to integrate amp-bind with the “Leave a Reply” heading in the comment form. Because WordPress only provides a comment_form_defaults filter for the comment_form() function, and not a filter like comment_form_args to modify the actual args passed in, we cannot use the comment_form_defaults filter to inject amp-bind logic into the form.

Longer-term solutions would be to introduce the comment_form_args filter to core, or even better to not use amp-bind and instead use amp-script for this.

The effect of this PR is that the “Leave a Reply” heading will remain unchanged in Twenty Nineteen and Twenty Twenty when you enter a comment or click a “Reply” link in the comments list. In Twenty Seventeen, which does not pass any arguments to comment_form(), the heading will change from “Leave a Reply” to “Leave a Reply to John Smith” when clicking the reply link. In all themes, the “Cancel Reply” link should still appear regardless of the heading changing.

Additionally, this PR prevents the amp-bind warning message from appearing:

Default value () does not match first result (Leave a Reply) for <SPAN [text]="commentform_post_2013.replyToName ? "Leave a Reply to " + commentform_post_2013.replyToName + "" : "Leave a Reply"">. We recommend writing expressions with matching default values, but this can be safely ignored if intentional.

Fixes #2489.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@laurelfulford
Copy link

Thanks Weston!

I've tested this PR with Twenty Nineteen, and with the Newspack theme and our plugin that renames comment labels. In both cases, it worked like a charm -- the duplicate title no longer happened, and the custom text you can add with the Rename Comments plugin was respected 👍

@westonruter westonruter requested a review from pierlon March 17, 2020 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comment form “Leave a Reply” heading misbehaves in Twenty Nineteen
4 participants