Issue status update for http://drupal.org/node/23584 Post a follow up: http://drupal.org/project/comments/add/23584 Project: Drupal Version: cvs Component: theme system Category: tasks Priority: normal Assigned to: Bèr Kessels Reported by: Bèr Kessels Updated by: Bèr Kessels Status: patch @steef: this is a /replacement/ for theme_box. @djnz eldarin; Cabn you please get me some figures? how many % of a pure HTML page are classes and IDs? how many BW will be consumed by a lot of classnames? how does this compare to an image? the drupal logo on drupal.org is ~4.3K, which means 3745 chraracters. with an average classname length of 6 characters that would mean 625 "classnames". In other words: You need to return 625 times a classname to get the same BW as a single very small logo. So, unless you can convince me with measurements and figures, I will noth bother about BW. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Wed, 25 May 2005 12:34:48 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/theme_wrapper.patch (1.23 KB) I finally got around this. On the useability sprint inAntwerp we agreed that theme_box is: 1) not flexible, 2) underused 3) very powerfull for themers, consistancy and developers if it has enough information. This patch introduces such a function, called theme_wrapper. Theme('wrapper') should be called EVERYWHERE where we now have hardcoded spans and divs, only to be able to add a class or ID to a (part of -) a page.
From the notes:
" ** HTML themed wrapper ** Currently, we use a theme_box to wrap parts of pages in HTML. This should be extended, but most of, all, get better CSS support. Thus a wrapper should be introduced, one that should replace theme_box. This theme_wrapper can and/or will get an ID and a class. * Both class and ID should be passed as elements, so that it is up to the developers to pqss correct classes and Ids; * Both the ID and class are required. (?) * Discuss whether or not they are required. **List of classes.** When passing a class, the developer should be encourqged to choose an existing class, instead of inventing new ones for every wrapper. This will be achieved by a list of standard classes, that will be available in drupal.css. Think of classes such as “3-col-left”, “3-col-center” and “3-col-right”, for wrappers that will make content in a 3 columned way. This could be used, for example in a directory.module to present three directories next to one another. TODO: Create a standard set of classes, that can, and should be, (re-) used by developers. ** Ids** Ids should be unique on a page, thus the developer must make sure, he or she does not use an ID that is already existing on the page where his or her HTML will be rendered. But just like with the classees, we should avoid a wildgrowth of ids. Thus, good guidelines, including lists of examples, lust be written. TODO: Create guidelines for ID-ing elements in Drupal. " ------------------------------------------------------------------------ Wed, 25 May 2005 12:45:26 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/box_to_wrapper_core.patch (3.8 KB) ... and here is a patch for core that replaces all theme('box' calls with new wrapper calls. ------------------------------------------------------------------------ Wed, 25 May 2005 17:53:49 +0000 : drumm theme_wrapper.patch looks okay. The ids used in box_to_wrapper_core.patch are a bit vague I think we should avoid using numbers in ids unless they correspond to an actual id in the db- such as node-37 or comment-48. For example "comment-4" in the patch could be renamed to "comment-viewing-options." ------------------------------------------------------------------------ Wed, 25 May 2005 19:45:34 +0000 : Dries I vote against numbers too. Can't we use 'comment' instead of 'comment-1', 'comment-2', etc. Most of the calls to theme('box') are used to print status messages. Shouldn't we use drupal_set_message() instead? That would be a lot more consistent with other status messages and is more likely to catch the user's eye. Please do. One more revision and this can be committed. ------------------------------------------------------------------------ Thu, 26 May 2005 07:51:43 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/box_to_wrapper_core_0.patch (3.82 KB) Here is an updated patch. Dries,: i agree on what you say about the messages, but my plan is as follows: 1) get the wrapper in core 2) change *only* existing ox calls into wrappers (the two patches bring us here.) 3) create a list of standard classes. i started that already, will share it soon. 4) add a lot of wrapper calls, on places where html is hardcoded (a lot: i see nearly fifty of them in core) 5) replace some wrapper calls by more appropriate calls, like the messages. But some things should go to hook help too. 6) compile/extract a list of IDs Step 3 and 4 will get us where you want, but for now i did not yet want to change to much. This issues and these patches are about *introducing* the new wrapper, not about *using* it, yet. attached the patch for core modules, with improved ids. ------------------------------------------------------------------------ Thu, 26 May 2005 07:54:26 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/theme_wrapper_0.patch (1.22 KB) and the patch for theme.inc. I changed the doxygen about IDs a little. ------------------------------------------------------------------------ Thu, 26 May 2005 19:13:58 +0000 : Dries I wonder why we need a 6-step plan to get such simple changes implemented ... ------------------------------------------------------------------------ Fri, 27 May 2005 15:25:39 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/theme_wrapper_1.patch (5.02 KB) The new patch. and no, the six step plan reaches beyond this patch, far beyond it. Getting this patch in is only one step. I only put that plan there to show why i am reluctant to add new features in this patch. IMO adding new features is for after this wrapper is in core. That is what i wanted to tell with that plan. ------------------------------------------------------------------------ Fri, 27 May 2005 15:30:54 +0000 : Bèr Kessels *sigh* set_message does not return anything :) ------------------------------------------------------------------------ Sat, 11 Jun 2005 22:36:20 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/theme_wrapper_1_0.patch (4.99 KB) another attempt ------------------------------------------------------------------------ Mon, 18 Jul 2005 20:42:14 +0000 : Bèr Kessels steven had some very good cons for this patch: ~There is no default for param 1 and 4 in other words, you always need to pass 2 and 3!! so, we really need to re-think the API and the backwards compatibility. I desiged it so that any changed would be minimal, but maybe a patch that will keep theme_box, as wrapper, wold suffice too. . ------------------------------------------------------------------------ Tue, 19 Jul 2005 21:23:37 +0000 : Dries The changes to the comment.module no longer apply. The changes to the search module are incorrect; part of the code is missing? Sometimes the ID and class are identical, sometimes they are not? Does it make sense to specify an empty title? Sure that should be an optional paramter? If you change the API, the following files will need to be updated: article/article.module banner/banner.module cvslog/cvs.module eatlocal/resource/resource.module ecommerce/cart/cart.module evaluation/evaluation.patch event/event.module filestore2/filestore2.module lanparty/lanparty.module location/location.inc location/location.module mail_archive/mail_archive.module massmailer/massmailer.module paypal_framework/paypal_framework.module peoplesemailnetwork/peoplesemailnetwork.module project/comment.inc project/project.module project/release.inc queue/queue.module recipe/recipe.module rsvp/rsvp.theme tagadelic/tagadelic.module term_statistics/term_statistics.module trackback/trackback.module troll/troll.module volunteer/volunteer.module Quite a few modules incorrectly use theme('box') as a substitute for drupal_set_message(). ------------------------------------------------------------------------ Wed, 20 Jul 2005 10:56:20 +0000 : stefan nagtegaal I'm not sure, but shouldn't we warp everything which is returned to the screen through theme_box()? Then it would be very easy to style particular tables/whatever differently from page to page... Just think about it, and consider this.. ------------------------------------------------------------------------ Wed, 20 Jul 2005 10:57:29 +0000 : stefan nagtegaal Something i forgot to mmention is that I would encourage a namechange from theme_box() to theme_wrapper() or theme_container(), just to make it bore obvious what it does.. ------------------------------------------------------------------------ Fri, 22 Jul 2005 21:11:42 +0000 : stefan nagtegaal Ber, Dries what do you guys think of this? ------------------------------------------------------------------------ Sat, 23 Jul 2005 01:53:19 +0000 : djnz I am concerned about bandwidth with the potential for many long class names/ids in both the HTML and CSS. Do you know how much effect this change will have? IMHO it would be better NOT to call theme('wrapper'..) at all, but to make the raw data available to the template engine - you can call it in the template if you want to, but if it is not right for your theme (eg handheld display) you don't need to. Of course this will be much easier once you move away from XTemplate. ------------------------------------------------------------------------ Sat, 23 Jul 2005 02:37:50 +0000 : eldarin djnz said: "IMHO it would be better NOT to call theme('wrapper'..) at all, but to make the raw data available to the template engine - you can call it in the template if you want to, but if it is not right for your theme (eg handheld display) you don't need to. Of course this will be much easier once you move away from XTemplate. " I wholeheartedly agree. Making the raw data available is the best. Whatever presentation details are best handled in the theme templating engine. I beg to differ on the fact that you need to move away from XTemplate to do this. I use the april 2005 version 1.7 of XTemplate which uses iterations/interpolations etc. over variables, and it's perfectly capable of handling the raw data into variables. The /xtemplate.engine/ interface is what's not exactly "optimal" with regards to handling flexibilty in themeing. The template-file /xtemplate.xtmpl/ is even worse with regards to flexibilty. The actual *XTemplate class* is just perfect - and extremely fast. I am completing my first version of an interface for the newer XTemplate where sub-blocks/subsections can override assigns done in parent-blocks/-sections in a way just like CSS declarations and where subsection logic can affect inclusion of parent-blocks/-sections. Xtemplate is extremely flexible and simple in my opinion; that it's probably the fastest (?) full-fledged templating system around doesn't hurt as well.