[drupal-devel] [task] Introduce theme wrapper,
as discussed on the Sprint
Dries
drupal-devel at drupal.org
Tue Jul 19 21:23:41 UTC 2005
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: Dries
Status: patch
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().
Dries
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.
.
More information about the drupal-devel
mailing list