[drupal-devel] [task] improve theme system
stefan nagtegaal
drupal-devel at drupal.org
Sun May 1 08:02:49 UTC 2005
Issue status update for http://drupal.org/node/21517
Project: Drupal
Version: cvs
Component: theme system
Category: tasks
Priority: normal
Assigned to: stefan nagtegaal
Reported by: stefan nagtegaal
Updated by: stefan nagtegaal
-Status: fixed
+Status: patch
Well, I added the wrapper drupal_get_help() to get more inline with the
rest of the theming function like drupal_get_title() and
drupal_get_breadcrumb()..
To get rid of the most PHP-logic inside the theme function itself, was
also a decision to make the warapper function. That way it is just one
call to the drupal_get_help() function to get the help, without having
to much attention to PHP at all..
I still think that my code is a little better when we look at
consistency and usability..
I set the status of this post to 'Patch' again because I would like to
improve the theme system more, by eliminating most of the PHP
code/logic out of the themable functions (by replacing the code in
wrapper functions), and so this is e-mailed to the drupal developers
list.
stefan nagtegaal
Previous comments:
------------------------------------------------------------------------
April 28, 2005 - 18:21 : stefan nagtegaal
The theme system is very inconsistent and not clean. In a steady stream
of patches i'll try to make the theme system clean and let most of the
theme functions behave the same..
One of the main goals is, that when writing themes in PHPTemplate for
example, the way how things are being returned is different from theme
function to theme function..
Let's try to unify this, so theming will be more efficient, simpler and
more consistent..
------------------------------------------------------------------------
April 28, 2005 - 18:40 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/cleaner_themable_help.patch (1.99 KB)
The attached patch does:
- remove unused function theme_help() and theme_error();
- added a wrapper function drupal_get_help() inside common.inc (for
consistency with drupal_get_title(), drupal_get_breadcrumb(), etc);
- reimplemented theme_help(), only this time for a more consistent way
for themers;
This patch makes it easier to theme the page specific helptexts for
drupal.
To change your PHP-based themes, you can do:
<?php
- if ($help = menu_get_active_help()) {
- $output .= '<small>'. $help .'</small><hr />';
- }
+ $output .= theme('help', drupal_get_help());
?>
To change your PHP-Template based theme:
<?php
if ($help):
?>
<?php
print $help;
?>
<?php
endif;
?>
becomes a simple:
<?php
print $help;
?>
If this patch is accepted i'll try to make the functions for returning
the local tasks and the status messages behave the same..
Overriding the <div class="help">...</div> could be done through
deifinig your own theme_help() function in your theme or template.php
file.
------------------------------------------------------------------------
April 28, 2005 - 18:44 : dikini
+1 from me
it makes help text themeable
it uses semantic markup, which is good practice, or as some say the
Right Way
------------------------------------------------------------------------
April 28, 2005 - 19:00 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/cleaner_themable_help_0.patch (1.99 KB)
The patch had DOS line endings and wouldn't apply.
I fully support this patch. +1
------------------------------------------------------------------------
April 28, 2005 - 20:38 : adrian
I completely +1 this.
------------------------------------------------------------------------
April 28, 2005 - 23:07 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/phptemplate_drupal_get_help.patch (816 bytes)
Because Adrian +1's this, his reward is a proper patch against
PHPTemplate, sorry I forgot this one...
here it is, a really simple one liner which does the job...
<?php
Index: phptemplate.engine
===================================================================
RCS file:
/cvs/drupal/contributions/theme-engines/phptemplate/phptemplate.engine,v
retrieving revision 1.24.2.1
diff -u -F^f -r1.24.2.1 phptemplate.engine
--- phptemplate.engine 23 Apr 2005 01:04:53
-0000 1.24.2.1
+++ phptemplate.engine 28 Apr 2005 21:03:59 -0000
@@ -219,7 +219,7 @@ function phptemplate_page($content) {
'tabs' =>
theme('menu_local_tasks'),
'messages' => theme_status_messages(),
'layout' => $layout,
- 'help' =>
menu_get_active_help(),
+ 'help' => theme('help',
drupal_get_help()),
'styles' => theme_get_styles(),
'mission' => $mission,
'is_front' => $frontpage,
?>
------------------------------------------------------------------------
April 28, 2005 - 23:35 : walkah
looks good to me. +1
------------------------------------------------------------------------
April 28, 2005 - 23:45 : chx
+1.
------------------------------------------------------------------------
April 29, 2005 - 10:53 : Bèr Kessels
and another +1
------------------------------------------------------------------------
April 29, 2005 - 21:12 : Dries
Any particular reason we can't do:
function drupal_get_help() {
return menu_get_active_help();
}
instead of the proposed:
function drupal_get_help() {
if ($help = menu_get_active_help()) {
return $help;
}
}
------------------------------------------------------------------------
April 30, 2005 - 04:12 : TDobes
Why do we need a drupal_get_help function at all? Why could the
theme_help function not look like this:
<?php
function theme_help() {
if ($help = menu_get_active_help()) {
return '<div class="help">'. $help .'</div>';
}
}
?>
This would be far more consistent with what we've done with status
messages (theme_status_messages). It would also ask less of theme
authors as they would only need $output .= theme('help'); instead of
$output .= theme('help', drupal_get_help());.
------------------------------------------------------------------------
April 30, 2005 - 11:29 : Dries
Isn't it amazing that everyone nods '+1' without actually reviewing the
code?
------------------------------------------------------------------------
April 30, 2005 - 12:05 : TDobes
Attachment: http://drupal.org/files/issues/cleaner_themable_help-20050430.patch (4.88 KB)
I've attached a "counter-patch" using the code I suggested. It also
includes patches to update the core themes, which were missing from the
other patch.
I also fixed the call to theme('status_messages'), which was being
called as the (non-themable) theme_status_messages.
------------------------------------------------------------------------
April 30, 2005 - 19:47 : Dries
Committed TDobes' latest version! Thanks.
More information about the drupal-devel
mailing list