The current comment-wrapper.tpl is pretty simple.

There are a couple issues we should at least discuss before D7 is out though.

  1. The list of comments and the comment form are mushed together into one $content variable. We should split them apart.
  2. There is no heading for the list of comments it contains. So users go straight from the node content straight into the comments list with any introduction into what is going on. A <h2><?php print t('Comments'); ?></h2> might be nice.
  3. Unfortunately, #2 leaves us with a sticky problem. Forum module uses comments, but prefixing forum's comments with a Comments H2 would be bad. One solution would be to fix #279573: Themes can't use node-story.tpl.php without node.tpl.php. Another solution would be the crufty solution we are attempting for Garland: #295895: Garland overrides overrides template with theme function
CommentFileSizeAuthor
#16 comment-wrapper-tweak-517850-16.patch954 bytesjohnalbin
#12 comment-wrapper-tweak-517850-12.patch1.03 KBjohnalbin

Comments

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Issue tags: +tpl-refresh

Adding issue tag

michelle’s picture

michelle
she/her
Primary language English
Location Wisconsin, USA
commented

This may be a little "out there" but how about a string in the UI per content type? Default it to "Comments" (translated) and you can wipe it out to not have anything. Could default the forum node type to not have anything and then other types that get added to the forum vocab need to be taken care of by the site admin.

Michelle

kika’s picture

kika commented

Why not introduce comment-*nodetype*.tpl.php and comment-wrapper-*nodetype*.tpl.php and overwrite comments header in forum-specific comment templates?

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

@kika Yep, that's what #279573: Themes can't use node-story.tpl.php without node.tpl.php would allow us to do. I think its the cleanest solution. :-) Unfortunately, its an outstanding bug that has not been fixed in a year and a half. :-(

@Michelle That's not a bad solution either. But I'm always hesitant to add more checkboxes to core. I don't quite see "adding a comment heading" as a compelling reason for more UI. :-\

I just remembered that comment-wrapper.tpl doesn't have a $node variable like the comment.tpl does. That would probably be useful. So let's make that #4 on the to-do list.

michelle’s picture

michelle
she/her
Primary language English
Location Wisconsin, USA
commented

@JohnAlbin: I wasn't thinking of a checkbox. I was thinking more akin to being able to name the "body" field or delete contents of the textbox entirely to not have one. In this case, you would give the name of the "comments header", which would default to "Comments", or delete the word "Comments" to not have a header at all.

Michelle

j_shb’s picture

j_shb commented

Concerning issue #2:

I'm new-ish to Drupal, but here's an issue I've run across. Even when there are no comments on a node, the comment-title heading (in my case, "COMMENTS") is shown. The reason for this in my comment-wrapper template is that the conditions to print the title are (1) the existence of $content (usually true because the comment form is included in $content) and (2) the node-type isn't a forum. Both of these are met for most of my nodes, regardless of whether or not there are no comments. Therefore, I've added another condition (maybe improper) but it hides "COMMENTS" when there aren't comments.

<?php if(substr($content,0,17) != '<div class="box">'): ?>
  <h2 id="comments-title"><?php print t('Comments'); ?></h2>
<?php endif; ?>

With this code, if the first thing in $content is the opening div for the comment form, the title "COMMENTS" doesn't print. Thought I'd share.

kika’s picture

kika commented

There are references to depreciated collapsed comments display modes in comment-wrapper.tpl.php. #506218: Remove collapsed comments and tidy up settings was meant to fix this but that template got missed.

yched’s picture

yched commented

[edited out, was talking nonsense]

yched’s picture

yched commented
Status: Active » Closed (duplicate)

Actually, it turns out this needs to be part of #523950: Update comment rendering. Patch #35 takes care of that - quite easy change once the way is paved by the other refactorings in there.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Closed (duplicate) » Active

Re-opening since I see what appears to be a superfluous <div> in comment-wrapper.tpl.

yched’s picture

yched commented

Note that the div around the 'add new comment' form (that's the one you're talking about, right ?) was already present before #523950: Update comment rendering, we just mirrored the existing markup.
But true, that could probably be enhanced. Should be much easier now that what was scattered across several theme functions is now inside a single template :-)

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Active » Needs review
StatusFileSize
new comment-wrapper-tweak-517850-12.patch1.03 KB

But its really nice to see all 3 of the points I brought up in the issue description to already be fixed. Yay, team!

So the minor things I see that still need cleanup are:

  1. the <div> around $content['comment_form']. The form already has a wrapping div, so the one in the tpl is superfluous.
  2. The <h2 class="comments"> and the <h2 class="title"> need some work. I feel like the h2 above the actual comments should have the "title" class and the h2 above the form should have something like "title comment-form".

Anybody else see anything?

Here's a patch that does #1 and #2.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

lilou commented
Status: Needs work » Needs review

HEAD is broken.

moshe weitzman’s picture

moshe weitzman commented

The number/distribution of exceptions is surprising. Bot error?

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
StatusFileSize
new comment-wrapper-tweak-517850-16.patch954 bytes

I also noticed that the "Comments" title is still printed even if there are no comments and we are only printing the "Post new comment" form.

Er… "post"? We're not using that term anymore in other places in core. We should change that title to "Add new comment".

To review, this patch:

  1. Prevents the "Comments" title from appearing when there are no comments and we are only printing the new comment form.
  2. Changes the class on the h2 around the "Comments" title from "comments" to "title".
  3. Changes the new comment form's title from "Post new comment" to "Add new comment".
  4. Adds a "comment-form" class to the h2 around the new comment form's title.
  5. Removes the superfluous <div> around the new comment form.

With this patch, this is how you differentiate between node, comment-wrapper, comment form, and comment titles:
node title: .node h2.title
comment wrapper title: .comment-wrapper h2.title
comment form title: .comment-wrapper h2.comment-form
comment title: .comment h2.title

Since the comment form title still has the "title" class, you'll also be able to style all h2 titles using h2.title.

robloach’s picture

robloach
he/him
commented

I like this patch. Another issue touching the same code is #539216: Comment form above comments.

effulgentsia’s picture

effulgentsia commented
Version: 7.x-dev » 8.x-dev

Darn. I like this patch, but seems too late now for D7. Correct me if I'm wrong.

moshe weitzman’s picture

moshe weitzman commented
Version: 8.x-dev » 7.x-dev

it is not a compatibility break, and it brings string consistency ("Add new comment"). IMO is eligible for D7

moshe weitzman’s picture

moshe weitzman commented

#16: comment-wrapper-tweak-517850-16.patch queued for re-testing.

effulgentsia’s picture

effulgentsia commented
Status: Needs review » Reviewed & tested by the community

John, Rob, and I like it, and Moshe says it's eligible, so RTBC: let's see what webchick says.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.