This patch changes the theme_box function. Currently we lak the ability to style blocks different, since we cannot add classes or IDS.
I fixed the theme_box to follow the system we use for sideblocks. The id and classes should be provided by arguments.

I have only one problem: backwards compatibility. To output valid XHTML all calls of this function must add a unique ID, I documented how this must be done, thats not the problem. The problem, is that we do not do this atm.

So untill all calls are fixed, I do:

theme_box($title, $content, $class = NULL, $id = NULL)

and not 

theme_box($title, $content, $class, $id)

which would be correct, since only the later will force developers to use and ID, resulting in always validating XHMTML.

I will try to generate a grep list with places where theme box is called and where it should be fixed.

Bèr

CommentFileSizeAuthor
#6 theme_box_0.patch1.22 KBBèr Kessels
theme_box.patch1.24 KBBèr Kessels

Comments

tangent’s picture

tangent commented

XHTML does not require a div to have an ID attribute. Why would an ID be required in this case?

Bèr Kessels’s picture

Bèr Kessels commented

XHTML does not require a div to have an ID attribute. Why would an ID be required in this case?

Because i do not wat to add an if($id) { } or even an $id ? 'id= etc.

That would be loads of ifs that are not needed: its much better to enforce everyone to add an ID. Also, if we make ids optional, a lot of lazy developers will not add it, resulting in a lot of brokenish themes. I perfer to have always an ID, and put tit in the coding guidelines, then to have only sometimes an id.
And the last reason is, that with the current useability discussions going on, there are people starting to look at javascripts in their themes. We should be ahead of that, and simply give all elements an ID. So that themers can add their javascript to do fancy stuff to parts of the pages. (like hide and show them using cards and or accordeons)

tangent’s picture

tangent commented

I see where you're going but I don't think that we should necessarily require *all* blocks to have an id. If a module developer doesn't want his block to have an id and prefers to have a set of blocks with the same or different classes I see nothing wrong with that. Requiring an ID seems a bit heavy handed.

This is really another issue but how will user definable modules be handled? I assume the module would just use an incremented ID?

chrisada’s picture

chrisada commented

if we make ids optional, a lot of lazy developers will not add it

As one who theme Drupal quite a lot, I am absolutely with Bèr on this point.

The deveoper might not need the id to be there, but there will be peoples who want them. Right now there is just too many things in Drupal that is impossible to style due to lack of CSS id. (classes we have plenty)

Bèr Kessels’s picture

Bèr Kessels commented

I cannot see the issues with "requiring" an ID. Really: is that so hard to do? A module might have one, or two, sometimes three blocks. Why cant you add two letters to your code for a unique ID?
Its all about consistency. I already know a huge lot of places in the code where the *block theme function iteself* should have been used, but is not. This is annoying, not for developers, they don't care. But for both users, who see inconsistent interfaces, and for themers who go like: Hey: I am sure I had all titles of all pages set to bleu, why is that one red? --aah. f*c, its an h3 that is not in a block, grrr.

Same will go for blocks ids. I do not know why, but the general html-code and interfaces of contributions are plain bad, in drupal. Core is nice, but a lot of contributions are hacks-upon-hacks, to keep up with the changing drupal-core versions. We should enforce developers to at least keep consistancy.

Bèr Kessels’s picture

Bèr Kessels commented
StatusFileSize
new theme_box_0.patch1.22 KB

This one has all parameters required.

drumm’s picture

drumm
he/him
Location NY, US
commented

The html outputting could be cleaned up a little, using single quotes where possible and not putting variables inside strings.

Also, I think this sohuld be renamed to theme_page_section() and it should not necessarily actually draw a box. Whether something has a literal box or not should be up to the themer on a case-by-case basis. I see this themeable function as omething to break pages up into areas and not add boxes.

Otherwise, I like where this is going and would like to see the code changed to call this properly. For now I won't give a whole +1, but it deserves at least +0.5.

adrinux’s picture

adrinux commented

Personally I find the 'every element with a class and id' style of drupal slightly annoying. It seems to me there is great emphasis on keeping the php code clean and effecient, but when it comes to xhtml a 'kitchen sink' include everything that a theme designer might possibly need and somewhat bloated approach prevails.

To me it all adds clutter that makes designing drupal themes harder not easier, especially for newcomers faced with a huge array of classes and ID's and not knowing quite what to use, or where they come from. Documenting them helps but not a lot.

That said I'm for anything that lets me over-ride xhtml and add my own in phptemplate :)

I'm not conviced that enforced ID's on all boxes is a good idea...

Bèr Kessels’s picture

Bèr Kessels commented

The reason why we would need IDS on each box, would be because of issues like Form groups should be collapsible.
so unless we introduce other theme_ or apis for such cases, the boxes will need more and better information.

Besides that, with this patch you will be able to use:
.box {} /*for any box*/
.meaningfull-class {} /*Developers can and should use classes from general lists*/
#modulename-idname {} /*if you want advanced power*/

But you are (1) not enfirced to use them, and (2) able to define general classes. This is still future, and cannot be enforced with code. But what we need is a meaningfull list of classes, a developer can choose his from. Examples would be:
.left2col
.right2col /* boxes that will show up as two columns in the main area*/
.important
.top /* boxes used to explain what that page does, for example the aggregator top-box */
.help /* boxes that will render as helptext, for example search help */
etc.

but again, thats for future, first we need the ability to "überhaupt" set these classes.

Bèr

Bèr Kessels’s picture

Bèr Kessels commented

superceded by http://drupal.org/node/23584