Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#20469 closed enhancement (fixed)

Twenty Eleven: better closed comments notes

Reported by: iandstewart's profile iandstewart Owned by: lancewillett's profile 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 12 years ago.
20469.2.diff (1.1 KB) - added by obenland 11 years ago.
20469.default-comment-status.diff (1.1 KB) - added by obenland 11 years ago.
20469.3.diff (1.1 KB) - added by obenland 11 years ago.
20469.3.twenty-twelve.diff (1.1 KB) - added by obenland 11 years ago.
20469.diff (3.2 KB) - added by kovshenin 11 years ago.
Includes twentyten, twentyeleven and twentytwelve
20469.4.diff (1.9 KB) - added by SergeyBiryukov 11 years ago.
20469.5.diff (1.9 KB) - added by SergeyBiryukov 11 years ago.

Download all attachments as: .zip

Change History (29)

#1 @nacin
12 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.

@obenland
11 years ago

#2 @obenland
11 years 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?

#3 @obenland
11 years ago

  • Keywords needs-testing added

#4 follow-up: @kovshenin
11 years 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.

#5 in reply to: ↑ 4 ; follow-up: @obenland
11 years 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.

#6 in reply to: ↑ 5 @kovshenin
11 years 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.

@obenland
11 years ago

#7 @obenland
11 years ago

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

#8 @lancewillett
11 years 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.

#9 @lancewillett
11 years 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.

#10 @nacin
11 years 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?

#11 @kovshenin
11 years 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 :)

@kovshenin
11 years ago

Includes twentyten, twentyeleven and twentytwelve

#12 follow-up: @kovshenin
11 years 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?

#13 in reply to: ↑ 12 @lancewillett
11 years 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.

#14 @lancewillett
11 years 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.

#15 @lancewillett
11 years 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.

#16 @lancewillett
11 years 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.

#17 follow-up: @SergeyBiryukov
11 years 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 11 years ago by SergeyBiryukov (next)

#18 @SergeyBiryukov
11 years ago

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

#19 in reply to: ↑ 17 @lancewillett
11 years 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.

#20 @lancewillett
11 years ago

In 22620:

Twenty Twelve: remove extraneous comment. See #20469.

#21 @lancewillett
11 years ago

In 22621:

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

Note: See TracTickets for help on using tickets.