WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 17 months ago

Last modified 17 months ago

#20469 closed enhancement (fixed)

Twenty Eleven: better closed comments notes

Reported by: iandstewart Owned by: lancewillett
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

Twenty Eleven currently shows a note about comments being closed when comments are closed on posts but not on pages that have comments. It also shows that note on posts that have never allowed comments. This could be improved by showing the note on posts AND pages but only if there have been comments made there in the first place.

Attachments (8)

better-comments-notice-20492.diff (1.2 KB) - added by iandstewart 2 years ago.
20469.2.diff (1.1 KB) - added by obenland 18 months ago.
20469.default-comment-status.diff (1.1 KB) - added by obenland 18 months ago.
20469.3.diff (1.1 KB) - added by obenland 17 months ago.
20469.3.twenty-twelve.diff (1.1 KB) - added by obenland 17 months ago.
20469.diff (3.2 KB) - added by kovshenin 17 months ago.
Includes twentyten, twentyeleven and twentytwelve
20469.4.diff (1.9 KB) - added by SergeyBiryukov 17 months ago.
20469.5.diff (1.9 KB) - added by SergeyBiryukov 17 months ago.

Download all attachments as: .zip

Change History (29)

comment:1 nacin2 years ago

Rather than using get_comments_number(), could we bring this inside the check above that uses have_comments()?

The one issue with using either get_comments_number() or have_comments() is that they both will count pings as a comment, when in reality the user may have only pings open but comments always closed. This isn't entirely uncommon; I've seen people try to push people to responding on their own blog and this would be a good way to do it.

obenland18 months ago

comment:2 obenland18 months ago

  • Cc lancewillett added
  • Milestone changed from Awaiting Review to 3.5

I really think this would be a great addition!

20469.2.diff moves message within the have_comments() conditional

20469.default-comment-status.diff adds an additional check to display the message only if users are supposed to be able to comment on posts in the first place.

@lancewillett: How do you feel about adopting this logic for Twenty Twelve as well?

comment:3 obenland18 months ago

  • Keywords needs-testing added

comment:4 follow-up: kovshenin18 months ago

20469.default-comment-status.diff seems to work well, though I feel weird that it doesn't display the "Comments closed" message, if there already are comments posted, but the "Allow people to post comments on new articles" option is disabled. 20469.2.diff works well.

comment:5 in reply to: ↑ 4 ; follow-up: obenland18 months ago

Replying to kovshenin:

I feel weird that it doesn't display the "Comments closed" message, if there already are comments posted

This wouldn't be "real" comments but rather just pings. See @nacin's comment above.

comment:6 in reply to: ↑ 5 kovshenin18 months ago

Replying to obenland:

That's true, however, if you disable "Allow people to post comments on new articles" you still have the option to allow commenting on a per-post basis, which can then be closed either manually or by the value set in "Close comments on old articles" so you end up with real comments (in addition to pingbacks, if any) but no "Comments closed" message, which creates the weird UX I'm referring to.

obenland17 months ago

comment:7 obenland17 months ago

Updated the patch to check for the existence of comment type comments. Which still works for the scenario Nacin mentioned.

comment:8 lancewillett17 months ago

  • Keywords needs-testing removed

In r22587: Twenty Twelve: better closed comments notes. Check only for "comment" type comments when deciding whether to show the message.

comment:9 lancewillett17 months ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 22588:

Twenty Eleven: better closed comments notes. Check only for "comment" type comments when deciding whether to show the message. Props iandstewart and obenland, closes #20469.

comment:10 nacin17 months ago

I think I'd prefer to take back my scenario if it means we avoid && ! empty( $comments_by_type['comment'] ) ) : in a default theme. Yuck, sorry I led you down that path. I could go for the original get_comments_number() check.

kovshenin, what exactly were you suggesting? Could you put it into patch form?

comment:11 kovshenin17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

My original concern was with:

'open' == get_option( 'default_comment_status' )

I'm okay with [22587] and [22588], but I agree $comments_by_type is not great for a default theme. I think we should replace that with get_comments_number, and also treat it as an int, not a string as in prior to [22587]. Patch incoming.

I also agree that displaying "Comments are closed" on a page with closed comments but with one or more pingbacks is a little weird too, but not really sure what can be done about that, since a pingback is essentially a comment, or a "thought" as we describe in some of the themes. We could also make use of pings_open() or maybe add an argument to get_comments_number() where we could specify the type of comments we want the number for, and use separate_comments internally. I don't know, pings are confusing, let's just deprecate them :)

kovshenin17 months ago

Includes twentyten, twentyeleven and twentytwelve

comment:12 follow-up: kovshenin17 months ago

In 20469.diff:

  • Checks get_comments_number() instead of using $comments_by_type in Twenty Eleven and Twenty Twelve, added get_comments_number() to Twenty Ten.
  • Loads the comments template in Twenty Twelve's single.php regardless of whether they're open or closed (consistent with page.php)
  • In Twenty Ten, moved the "Comments closed" block outside the ! have_comments() condition.

Also, for some reason, Twenty Ten defines the following styles:

.nopassword,
.nocomments {
	display: none;
}

Which makes the "comments closed" and "password protected" messages invisible. Not sure why it's doing it, but it doesn't feel safe to remove it. nocomments was invisible since the first import, nopassword was added in [15125] with a good reason.

Thoughts?

comment:13 in reply to: ↑ 12 lancewillett17 months ago

Replying to kovshenin:

Also, for some reason, Twenty Ten defines the following styles:

.nopassword,
.nocomments {
	display: none;
}

Which makes the "comments closed" and "password protected" messages invisible. Not sure why it's doing it, but it doesn't feel safe to remove it. nocomments was invisible since the first import, nopassword was added in [15125] with a good reason.

Yes, let's keep those rules. They avoid a duplicate message on output.

comment:14 lancewillett17 months ago

In r22615: Twenty Twelve: better fix for #20469 and comments closed messages, also load the comments template in single.php template regardless of whether comments are open or closed (consistent with page.php). Both props kovshenin.

comment:15 lancewillett17 months ago

In 22616:

Twenty Eleven: in comments template check get_comments_number() instead of using $comments_by_type for showing a comments closed message. Props kovshenin, see #20469.

comment:16 lancewillett17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 22617:

Twenty Ten: move the "Comments closed" text outside the have_comments() conditional, props kovshenin. Closes #20469.

comment:17 follow-up: SergeyBiryukov17 months ago

Nitpicking: [22617] removed the brackets in line 71:

<?php endif; // end ! comments_open ?>

This is inconsistent with line 73, and it's not obvious at a glance that comments_open is a function. Could we add the brackets back?

<?php endif; // end ! comments_open() ?>
Version 0, edited 17 months ago by SergeyBiryukov (next)

SergeyBiryukov17 months ago

comment:18 SergeyBiryukov17 months ago

20469.4.diff also removes a space after the string in all three themes.

comment:19 in reply to: ↑ 17 lancewillett17 months ago

Replying to SergeyBiryukov:

Nitpicking: [22617] removed the brackets in line 71:
This is inconsistent with line 73, and it's not obvious at a glance that comments_open is a function. Could we add the brackets back?
Perhaps the comment can even be removed (Twenty Eleven and Twenty Twelve don't have it).

Eh, OK. I'll fix it up.

comment:20 lancewillett17 months ago

In 22620:

Twenty Twelve: remove extraneous comment. See #20469.

comment:21 lancewillett17 months ago

In 22621:

Twenty Ten: clean up r22617, remove extra end comment and indent code. See #20469.

SergeyBiryukov17 months ago

Note: See TracTickets for help on using tickets.