Closed (fixed)
Project:
Drupal core
Version:
6.x-dev
Component:
forum.module
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
9 Dec 2005 at 18:26 UTC
Updated:
13 Jul 2007 at 07:51 UTC
Jump to comment: Most recent file
Comments
Comment #1
Wesley Tanaka commentedComment #2
dries commentedCoding style needs work; we don't glue word together + incorrect use of spaces.
Comment #3
Wesley Tanaka commentedhuh?
I split the css classes with hyphens if that's what you meant by the first part
I removed the extra space that was previously after $description
I added spaces between the . concatenation operator and the $class variable
But I'm just kind of grasping at straws here, since i don't understand your comment.
Comment #4
Wesley Tanaka commentedComment #5
Tobias Maier commentedI never looked at the whole function you are patching here
but do we really need the class "no-new-topics"?
if it is not new it is a ordinary topic so dont define any class
and you want to use padding...
when you think about the box-modell then margin-left would be better in my opinion...
Comment #6
Wesley Tanaka commentedpadding allows for a background image to use that space.
See
http://treehouse.ofb.net/go/en/forum
for an example (create an account/login to see the classes in action)
Comment #7
jose reyero commentedI agree with Thobias, we don't need new styles here.
And indeed, hardcoding styles in the HTML code is not a good idea. I'm talking too of these style="margin...", which I handn't seen before.
I think all that styling should be in the CSSs and for displaying the nested comments a set of enclosed divs would be more adequate. But for things like adding images, this a themeable function, so you can add that funny things in your theme.
Comment #8
Wesley Tanaka commentedDid you mean "we don't need a new class here"?
Good point about it being a themeable function. Let me offer up an extreme proposal using those arguments: Why not get rid of the new-topics class and lighten up the HTML?
The point is that having a new-topics class (but no non-new-topics class) is rather pointless the way CSS is today, because I don't think it's possible (and if it is, probably not very well supported) to do a CSS selector that picks all elements at a given level that *don't* have a certain class applied.
Please correct me if I'm wrong about that (especially the "very well supported" part).
Comment #9
Wesley Tanaka commentedI agree that this is the way the codebase should have already been. Perhaps it might be worth opening a new issue to clean up this particular theme function to make that the case?
To clarify -- this deals with the main forum page, which does not have any comments displayed on it -- only a summary of what forums are available.
Comment #10
heine commentedAs this function is themeable I consider this a feature request.
Comment #11
Wesley Tanaka commentedIf we were to extend that reasoning to a conclusion, we should strip any default formatting, classes, etc. out of core default theme functions, and force themes to override all theme functions, giving a very spartan look to a site that didn't override any theme functions.
There may be an argument for doing things that way, but currently, default theme functions are structured to provide a reasonable default which themes can often customize only using CSS.
The particular theme function in question -- theme_forum_list -- is a monolithic beast that appears to be written with the idea that the "class" attributes defined on the output it produces will be suitable for themeing. Note that almost all elements produced have a class defined. This just adds a class attribute to one of the few cases that didn't have one defined.
Comment #12
drummThis is a themeable function (a bit large and full of too much PHP, but still themeable), so the changes are not necessary for core.
There are two separate changes here.
1. Change margin to padding
- This would make the code inconsistent since there is a similar margin further down.
- I think the box model argument is better than the background image one. Especially since you can change it in your theme.
2. Add a class showing if there are new posts or not.
- This is relatively okay.
- You can always leave out the not-.. class and apply CSS that would be for not new to all of them and then layer the new class on top. Having not-... classes is not a convention in Drupal.
To solve both- why not put the class on the parent table cell and put your background there?
Comment #13
Wesley Tanaka commentedGreat idea.
I added the class to the table row instead of the cell, to allow for things like changing the background of the whole row.
Also included in the patch are classes for forum.css which use the forum new/default icons in misc/ as an example. They look reasonable with the 4 themes that ship with core, but I don't know how they will interact with other themes.
Comment #14
Wesley Tanaka commentedComment #15
Wesley Tanaka commentedThis patch is the same as in #13, but the colons in the CSS changes have spaces after them now.
Comment #16
drummif statements and other blocks should always have {}
Comment #17
Wesley Tanaka commentedadded curly braces to if block
Comment #18
dries commentedCommitted to CVS HEAD. Thanks.
Comment #19
(not verified) commentedComment #20
Wesley Tanaka commentedNote: the patch on #17 will apply against the 5.x forum module if anyone happens to be looking for a backport.