Themeable wrapper for all comments [#73748]

The comment module allows theming of each comment individually, but there is no way to wrap the entire comment section in its own div... comment_render doesn't allow for that.

Maybe comment_render can be updated to include a call for a themable wrapper around all the comments (default of course would be blank).

Then it will be posssible to address the entire node body and the entire list of comments as separate items on the page without resorting to template markup hackery.

Example of current output:

<div class="node">node body</div>
<div class="comment">comment 1</div>
<div class="comment">comment 2</div>
<div class="comment">comment 3</div>

Requested output:

<div class="node">node body</div>
<div class="comments">
  <div class="comment">comment 1</div>
  <div class="comment">comment 2</div>
  <div class="comment">comment 3</div>
</div>

Since page.tpl.php exposes only $content, it seems most appropriate that this feature should be provided in the comment module.

Comment	File	Size	Author
#15	comment-wrapper_1_1.patch	1.64 KB	RobRoy
#15
#9	comment-wrapper_1_0.patch	1.64 KB	RobRoy
#9
#8	comment-wrapper_1.patch	1.64 KB	rkerr
#8
#4	comment-wrapper_0.patch	1.64 KB	RobRoy
#4
#1	comment-wrapper.patch	1.71 KB	rkerr
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

rkerr CreditAttribution: rkerr commented 14 July 2006 at 20:32

Status:

Active

» Needs review

File	Size
comment-wrapper.patch	1.71 KB

Created a new function theme_comment_wrapper()

This is called after all the comments have been themed, at the end of comment_render() and puts a div with id="comments" as well as the anchor id="comment" around the already-themed comments.

Comment #2

m3avrck CreditAttribution: m3avrck commented 14 July 2006 at 21:23

I agree 100%. I ran into this problem on a previous project, this needs to be fixed.

The approach to this patch looks solid.

Comment #3

Tobias Maier CreditAttribution: Tobias Maier commented 15 July 2006 at 05:55

+1 for more flexibility and themability

is <a id="comment"></a>
really needed after this patch, because the div works as anchor, too...

in general I'm against introducing new ids in drupal, because we loose flexibility with them
--> maybe someone wants to show two full nodes with a module like dashboard, what he gets is an id conflict...

Comment #4

RobRoy CreditAttribution: RobRoy commented 15 July 2006 at 20:04

File	Size
comment-wrapper_0.patch	1.64 KB

We should be able to scrap the anchor as it is a duplicate ID. Here's a revised patch.

Comment #5

rkerr CreditAttribution: rkerr commented 17 July 2006 at 17:50

I kept the "comment" anchor because it was in the original function. I guess it is used somewhere for linking through to node/xxx#comment. But why it was using id instead of name I'm not sure.

We might need to retain some kind of anchor for jumping directly to comments.

Comment #6

rkerr CreditAttribution: rkerr commented 17 July 2006 at 17:51

Whether the wrapper around the comments has an id or class, I don't really have a preference either way.

Comment #7

RobRoy CreditAttribution: RobRoy commented 17 July 2006 at 18:13

Using a DIV with ID="comment" allows users to still go to page#comment. IIRC, in HTML 4 any page#whatever will go to any HTML element that has ID="whatever" so this will still serve that purpose and not serve up a duplicate ID.

Comment #8

rkerr CreditAttribution: rkerr commented 17 July 2006 at 18:56

File	Size
comment-wrapper_1.patch	1.64 KB

Yeah you're correct. :) Why hadn't I noticed that before?

OK, so I'd say for minimal disruption to the default core markup, remove the anchor and keep the same id="comment" on the wrapping div.

(attaching updated patch)

Comment #9

RobRoy CreditAttribution: RobRoy commented 17 July 2006 at 19:03

File	Size
comment-wrapper_1_0.patch	1.64 KB

Oops, thought that's what I had done. I updated your patch removing the unneeded backslash.

Looks good to me know and ready for review!

Comment #10

rkerr CreditAttribution: rkerr commented 17 July 2006 at 19:22

Oops. :) Thanks for updating that!
Let's see if we can get some eyes on this...

Comment #11

m3avrck CreditAttribution: m3avrck commented 18 July 2006 at 09:55

Not to nit-pic, but wouldn't it make more sense for that to be <div id="commentS"> ... make it plural, as there are many times I use class="comment" for individual comments.

Otherwise looks solid, about time that <a> was removed :-)

Comment #12

rkerr CreditAttribution: rkerr commented 18 July 2006 at 13:30

I think plural does make more sense, but wanted to keep the default id the same so that any other code that tires to link to node/id#comment will still work. (Without going on a find/replace in the rest of the modules).