[drupal-devel] [task] Introduce theme wrapper,
as discussed on the Sprint
m3avrck
drupal-devel at drupal.org
Tue Sep 13 18:24:58 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: m3avrck
Status: patch (ready to be committed)
I agree with Dries, although the wrapper is a good idea, it does
introduce extra markup that IMO would be better if we could avoid
entirely. If we can get rid of the other theme('box') I think that
would make the most sense.
Remember: KISS (keep it simple stupid) !
m3avrck
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.
------------------------------------------------------------------------
Sat, 23 Jul 2005 07:19:52 +0000 : Bèr Kessels
@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.
------------------------------------------------------------------------
Sat, 23 Jul 2005 09:34:26 +0000 : eldarin
Bèr,
I'm actually for the use of CSS, class-names, unique IDs etc. with
XHTML and all-liquid layouts tweaked into the most stunning creations
like on www.csszengarden.com [1], and the ability to support different
media platforms, cascading CSS rules and keeping IE, FF and the others
in standards-compliant mode - avoiding quirks mode for box-layout in
any browser platform and generally avoiding tag-soup...
... it's delegating the creation of XHTML structure outside the
template engine that I don't like.
The template engine I have nearly completed supports as much
flexibility I think I'd care to use in any near future with support for
creating the whole page with iterated theme wrappers for
node/comment/box/block still not requiring parsing the page until a
final recursive parse with cut-off of subsections/sub-blocks at this
time and all logic in assigning/altering variables done in the
invocation of section-dependent functions with varying granularity just
like CSS - i.e all variables are first setup and then one parse fills in
the variable fields and outputs the XHTML.
That means I could also archive or cache presentation-independent copys
of the web-pages using serialization - or possibly optimizing
page-serving by changing sub-sections of cached page from the
un-serialized content variables ... e.g changing just a
/my-forum-threads/ for each user-id on the front page - while avoiding
other content-pulling function calls or DB queries until cached page is
dirtied.
Well, that's some of the benefits I see with raw content data handled
in the template engine, although you misunderstood what I said in my
previous post.
:-)
[1] http://www.csszengarden.com
------------------------------------------------------------------------
Sat, 23 Jul 2005 09:34:48 +0000 : djnz
"@djnz eldarin; Can you please get me some figures?
"
I was kind of hoping *you* might come up with some figures to justify
the change, unless it is optional/themeable.
IMHO anything that results in a step change in either disk footprint,
execution time or bandwidth requirement needs to be carefully
considered. One of the themes I am working on is for handheld wireless
devices. There are no images and every byte counts - my whole payload
is less than 4.3k.
------------------------------------------------------------------------
Sat, 23 Jul 2005 09:43:43 +0000 : eldarin
Just as I mentioned the possible serialization of all content variables
- including iterated sections like node,comment etc.; I forgot to
mention the best part of caching a partially parsed page, with some
variable fields intact - e.g a personalized part of the page. That
would prune down the amount of parsing repeated for this special case.
There are some improvements done to XTemplate class to achieve this
capability of conserving all content - including iterated parts - but
nothing more than 3 lines of code in the parse-function.
------------------------------------------------------------------------
Sat, 23 Jul 2005 12:37:51 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/theme_wrapper_2.patch (5.48 KB)
Serialisation: Sounds like a good idea. But since there is no patch for
that yet, I suggest we god for the simple solution: centralise the HTML
wrapping. Which is what this patch does.
Dries:
"The changes to the comment.module no longer apply.
The changes to the search module are incorrect; part of the code is
missing?
"
There was a strange thing going on with that patch, this one is fixed.
"Sometimes the ID and class are identical, sometimes they are not?
"
Yes, they often are. Next to this wrapper patch, I want to get the
classes used in a better way: http://drupal.org/node/27316 will be
updated and published once this patch hits core.from there on we can
slowly get all the classes to be standardised.
"Does it make sense to specify an empty title? Sure that should be an
optional parameter?
"
I changed the order of the parameters. My intention was to make
theme_wrapper look like theme_box and theme_block as much as possible,
but indeed the empty title makes no sense; Fixed in the latest patch.
------------------------------------------------------------------------
Sat, 23 Jul 2005 12:52:28 +0000 : djnz
I guess the fact that it is an overridable theme_ function takes away
the objection about payload bloat. Just a couple of comments about the
function itself:
$class = NULL - should this be $class = '' or does "wrapper$class"
tolerate NULL?
Whilst I understand your reasons for wanting id's (I think) it is going
to be really hard to ensure they are unique. How about allowing $id = ''
as the default and then do
$id = $id ? " id=\"$id\"" : '';
$output = "<div class=\"wrapper$class\"$id>\n";
------------------------------------------------------------------------
Sat, 23 Jul 2005 13:01:44 +0000 : Bèr Kessels
About the classes: yes NULL is fine.
about IDs: on the sprint and the HIG meetings we agreed that id should
be mandatory. an ID needs to be only unique on one page, so really,
there should be no trouble. And in any case: haveing custom DIVs in
your code gives certainly no good way to determine the ids.
And a note about that additional BW: you should simply make your own
mobiletheme_wrapper() function if you do not want the id and class
details in your HTML.
------------------------------------------------------------------------
Tue, 02 Aug 2005 18:47:59 +0000 : Bèr Kessels
The patch still applies, with some offsets. It has been discussed and
reviewed a lot. But Id like the last patch to be reviewed one more
time, before we can set the status to 'to be committed'. Anyone? All
you themers out there, this will benefit you a LOT! please review!
------------------------------------------------------------------------
Thu, 04 Aug 2005 22:53:54 +0000 : eldarin
The patch makes a lot of sense to me, and my theme engine as it is now
will benefit. Having class and id fields along with content+title
instead of just the region is much better with regards to CSS styling.
I would of course prefer to be able to receive arrays as a little more
complex content, but that all depends on the modules ... :-)
I made my theme engine such that I can have multi-level loops and rules
"tagged" on theme-blocks (modified xtemplate v0.3), so that's why I
would like arrays as content instead of just 50 calls to theme_wrapper
on a list - e.g log listing.
Sometimes it's faster to just choose the right template file, than
having 10s of if-thens to decide what/how content should be parsed into
blocks ... Initially selecting template file based on url, user,
browser-sniffing, taxonomy etc. is then just a question of matching it
from a simple array.
Now, I just need to get my homebrew forum module up to speed. It now
supports deleting/moving sub-threads or posts (between threads),
filtering on users in threads etc. I think this theme_wrapper function
in the core would greatly ease designers' efforts as well as
developers' .
------------------------------------------------------------------------
Fri, 05 Aug 2005 09:02:06 +0000 : Bèr Kessels
"I would of course prefer to be able to receive arrays as a little more
complex content, but that all depends on the modules ... :-)
"
No. the idea of drupal is not to theme everything on one level, but to
theme things in a wzoom-out-like manner.
You start themeing the smallest widgets and elements, and so slowly
move up untill you theme the whole page.
If you do that correct, you have much more control, in a more
consistent way; with much less complexity then theming arrays and
complex datastructures.
Think of it liek a house: you can off course design the house by
designing 10.000 bricks inivdually in that house (the array), but its
much better to first design a standard brick, and a tile, and then
build the house with these bricks. (a page built with themed chunks)
------------------------------------------------------------------------
Sun, 21 Aug 2005 11:08:55 +0000 : Bèr Kessels
The patch still applies. its a really small patch, with a small impact,
yet it will give future themes a lot more power.
What is holding this so long? Can someone tell me what is not good
about this patch? Why it was not yet committed? I think it is really
silly not to get this in before 4.7.
------------------------------------------------------------------------
Mon, 22 Aug 2005 07:51:10 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/theme_wrapper_3.patch (7.17 KB)
Karoly pointed out to me that I had to patch phptemplate engine too. I
did so.
However, I cannot manage to use diff in such away that it removed and
adds files, the option -N does nothing here. (argh, I hate diff just as
much as cvs)
So please remove box.tpl.php and add the file in the next issue.
------------------------------------------------------------------------
Mon, 22 Aug 2005 07:52:08 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/wrapper.tpl.php (197 bytes)
and the new file. This must -of course- live in
themes/engines/phptemplate
------------------------------------------------------------------------
Mon, 22 Aug 2005 18:37:06 +0000 : Dries
How about updating the CSS files?
------------------------------------------------------------------------
Mon, 29 Aug 2005 16:59:51 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/theme_wrapper_4.patch (8.41 KB)
This is a patch that also fixes the two core themes that use a .box
class.
The wrapper.tpl.php is still not included, and must still be taken from
the post above.
------------------------------------------------------------------------
Mon, 29 Aug 2005 17:06:56 +0000 : Bèr Kessels
I am a bit reliucatant to set my own code to "ready to be committed",
but with the tight timeframe, the freeze etc, i think it is fair to do
so, now that Dries's comments are met. Please st this state back if you
disagree.
------------------------------------------------------------------------
Mon, 29 Aug 2005 21:04:16 +0000 : Bèr Kessels
Changed the menu CSS id generation, so to make sure we have unique ids.
Cleaned up some sloppy spaces and indentations.
------------------------------------------------------------------------
Mon, 29 Aug 2005 21:19:16 +0000 : m3avrck
Did you forget to attach this new file???
------------------------------------------------------------------------
Tue, 30 Aug 2005 19:20:12 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/theme_wrapper_4_0.patch (8.44 KB)
off course I did;
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:18:30 +0000 : Bèr Kessels
bump. is anything holding this patch from being committed?
------------------------------------------------------------------------
Tue, 13 Sep 2005 18:20:25 +0000 : Dries
I'm no longer convinced this is a good thing. There are only 5
theme('box')'s left in core, some of which are not strictly necessary
in my view:
* Unlike any other form, we wrap the comment form in a theme('box').
Why?
* Unlike any other table, we wrap the menu table in a theme('box').
Why?
* ...
I'd like to see us investigate if the remaining calls to theme('box')
could be removed. I'm reasonably convinced that we can.
More information about the drupal-devel
mailing list