This patch needs work: mostly it needs better documentation, and it needs more testing than I have given it.

I also believe that _forum_format() needs to be a theme function and this would be the appropriate place to do it.

One note: These .tpl.php files DO NOT USE theme('table') like the originals did. I discussed this with Dries and Drumm a few weeks ago, and theme('table') is a fantastic programmer tool but it is not accessible to themers. Therefore, these them functions construct their own table in a way that themers can modify. This makes it much easier for a themer to add and move columns around as they see fit.

CommentFileSizeAuthor
#26 forum.tpl__5d.patch12.72 KBdvessel
#25 forum.tpl__5c.patch12.28 KBdvessel
#23 forum.tpl__5b.patch7.35 KBdvessel
#21 forum.tpl__5.patch7.03 KBdvessel
#16 forum.tpl__4c.patch29.47 KBdvessel
#15 forum.tpl__4b.patch29.47 KBdvessel
#14 forum.tpl__4.patch29.5 KBdvessel
#12 forum.tpl__3.patch19.29 KBmerlinofchaos
#6 forum.tpl__2.patch28.97 KBmerlinofchaos
#3 forum.tpl__1.patch28.51 KBmerlinofchaos
#1 forum.tpl__0.patch28.61 KBmerlinofchaos
forum.tpl_.patch21.47 KBmerlinofchaos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs work » Needs review
FileSize
forum.tpl__0.patch28.61 KB

Ok, this version contains fairly significant documentation. This is a documentation style for .tpl.php files that I'm basically putting forward. It also requires that we put additional documentation (for the old-style theme_* functions so that they remain in the documentation tree) in to contributions/docs/developer -- perhaps we should create a theme.php and document these functions there, along with a @see.

Frando’s picture

Frando CreditAttribution: Frando commented
Status: Needs review » Needs work

Hm, in the comment in forum_display.tpl.php, you're writing that $forums and $posts may contain unsafe data, but several lines below, there's just print $forums, so it's printed without checking for unsafe data. This looks weird, and is certainly confusing for a designer.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
FileSize
forum.tpl__1.patch28.51 KB

Whoops! I forgot to go back and fix that comment. Good catch, Frando; the comment is wrong now. Here is a corrected version.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs work » Needs review
Dries’s picture

Dries CreditAttribution: Dries commented

I haven't tested this patch, but I looked at the code. I didn't saw anything that stood out as being bad. It would be great to see this patch go in. :)

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
FileSize
forum.tpl__2.patch28.97 KB

Gurpartap spotted a bug in this in his review and told me on IRC; It's quite minor -- if there are no forums, $variables['forums'] remains an array that gets output by the template, which is bad. It just needs an if to check if the array is empty and, if so, replace it with ''. Likewise, for safety, we should do the same with $variables['topics'].

I've also added an @see to functions that will be defined in theme.php in the contrib docs.

Gurpartap Singh’s picture

Gurpartap Singh CreditAttribution: Gurpartap Singh commented

Guess you need to add /* LTR */ comment after the margin-left: ...; in the CSS addition.

Dries’s picture

Dries CreditAttribution: Dries commented

I'd love to commit this, but: all other files in Drupal use dashes, not underscores. Is that something we can change so there is some consistency here?

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

We can. Though the user templates got committed as user_profile.tpl.php and user_profile_category.tpl.php and user_profile_item.tpl.php

Though it's actually fairly trivial to change them to dashes; I would worry only about minor confusion. I don't think it's that big of a deal, though.

I'll roll an alternate patch when I have some time.

Dries’s picture

Dries CreditAttribution: Dries commented

Yep, let's rename those other template files too and make everything nice, crispy and consistent. I think this is a very important patch for D6 and I encourage more patches like that. We should focus on popular theme functions first, of course. :)

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs review » Needs work

The odd/even classes are not surrounded by quotes and tableheader.js isn't included. Not sure that last one matters but theme_table() now automatically adds it when there are headers.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
FileSize
forum.tpl__3.patch19.29 KB

New patch changes underscores to dashes in TPL files. This is going to ripple a bit in the pattern-matching for the dynamic theme functions, where there's just one pattern but two different standards. We can probably have phptemplate.engine autotranslate the pattern when looking at files, though, but that'll require...another patch.

I'm not touching the user template patches here -- they need a lot more love than just renaming; they completely violate many of the points of the new system. I'll take care of that in a separate patch.

Quotes taken care of on the class. dvessel: tableheader.js is broken in IE. I suppose that doesn't matter here, but it is pretty bad. I'm not sure that tableheader.js is desirable on forums. Actually, I'm not sure tableheader.js is desirable on MOST tables. This may be a personal preference, though.

Goba: /* LTR */ comment added. Does there need to be a reflected line (i.e, padding-right) on the indent in forum-rtl.css? I'm not at all sure how this RTL stuff works.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

My next work is aggregator (half done already but paused cause Crell was working on fixing the very broken module), comment and book. Book needs to wait because pwolanin's patch is still out and I don't want to work on it until that patch is firmly in or out (btw I support the patch).

After that I'm not sure where the priorities are. Many modules either aren't themable or have not a lot to theme. I'm not too concerned with the administrative items; that's less important to theme, for the most part, and a lot of them just dump into theme('table') and that's fine for administrative stuff.

If anyone wants to talk more about priorities, go here: http://drupal.org/node/141914

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs work » Needs review
FileSize
forum.tpl__4.patch29.5 KB

Your last patch didn't include the .tpl files. I also renamed the files inside doxygen to point to dashed names.

Also added template suggestions for forum-display.tpl.php since it can output both containers and topic listings. Can be very useful for theming.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
FileSize
forum.tpl__4b.patch29.47 KB

Just being anal here.. added new line to forum-topic-navigation.tpl.php.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
FileSize
forum.tpl__4c.patch29.47 KB

Meh, one more time.. very minor.

btw, I'm working on user.module.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Good job! :)

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Fixed » Needs work

Actually, one thing.

s$ ls -als modules/forum/*tpl*
8 -rw-r--r--   1 dries  dries   823 Jul 22 09:01 modules/forum/forum-display.tpl.php
8 -rw-r--r--   1 dries  dries   752 Jul 22 09:01 modules/forum/forum-format-topic.tpl.php
8 -rw-r--r--   1 dries  dries   607 Jul 22 09:01 modules/forum/forum-icon.tpl.php
8 -rw-r--r--   1 dries  dries  2825 Jul 22 08:58 modules/forum/forum-list.tpl.php
8 -rw-r--r--   1 dries  dries  2367 Jul 22 09:01 modules/forum/forum-topic-list.tpl.php
8 -rw-r--r--   1 dries  dries  1275 Jul 22 09:01 modules/forum/forum-topic-navigation.tpl.php

We might want to massage the names of the template files a bit more? It would be nice if, by just looking at the name of the files, you'd be able to guess what they are about. Let's see if people have some suggestions for further improvements.

For example:

  • The word 'display' is redundant.
  • The word 'format' is redundant.

Just some thoughts. Let's try to optimize the discoverability a bit.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

I was thinking of changing "forum-format-topic" since it's not even a topic. It displays the authors name and date of the last post but does it even need to be its' own function? How about pushing that into forum-list.tpl.php?

I refrained from changing names since it's past code freeze and the function name should match the template name. Thought it was off limits.

For forum-display, how about just "forums"..?

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I actually added forum_format_topic, which wasn't originally themeable and was called _format_topic. It's used in 3 places, I think, so can't be JUST in one location. I couldn't immediately think of a better name. forum-author-info?

THe entire theme function can be easily renamed at this point, since it won't affect anything else.

forum-list as forums.tpl is fine.

I would advocate changing the theme function names as well so that they match.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs work » Needs review
FileSize
forum.tpl__5.patch7.03 KB

Changed forum-display to forums and forum-format-topic to forum-submitted.

Changed around template_preprocess_submitted a bit also so the variables $author & $time is always set. In one instance I changed how it's called so it doesn't force a NULL as I don't think it's necessary.

I don't know how to rename files through patches so all it does is create new files. Files with the older names will linger around so please remove.

Dries’s picture

Dries CreditAttribution: Dries commented

Looks like an improvement to me.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
FileSize
forum.tpl__5b.patch7.35 KB

Rolled again for latest HEAD.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Great. I've committed this patch. I still think there is some room for improvement, but for now, this is close to perfect. :-)

Good job, dvessel.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Fixed » Needs review
FileSize
forum.tpl__5c.patch12.28 KB

It was mainly the work of Merlinofchaos.

Here it is again, a few bugs were squashed with some minor tweaks.

Template naming suggestions was fixed. One extra template suggested and it makes sure the proper order is fed in.

Put the striping class into the variables function. All that row counting in the .tpl file had too much logic.

Set an isset() check for *->last_reply in template_preprocess_forum_list(). It's not always there.

Now checks for $forum->description before printing it's surrounding markup.

Instead of a for() loop inside forum-list.tpl.php to repeat the indent wrapper, now does print str_repeat('<div class="indent">', $forum->depth);.

Escapes all variables (is that what you call it?). All variables are set now.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
FileSize
forum.tpl__5d.patch12.72 KB

Forgot a few more variables that needed escaping.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Alright, all good work. Thanks. Committed.

Status: Fixed » Closed (fixed)

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

seangee’s picture

seangee CreditAttribution: seangee commented
Category: task » support

Hey guys - this looks great and just what I was looking for. Will it work with 5.2.x?

Thanx

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Not even a little bit!