improve theme system
Stefan Nagtegaal - April 28, 2005 - 16:21
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | Stefan Nagtegaal |
| Status: | closed |
Description
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..
#1
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:
<?phpif ($help):
?>
<?phpprint $help;
?>
<?phpendif;
?>
becomes a simple:
<?phpprint $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 owntheme_help()function in your theme or template.php file.#2
+1 from me
it makes help text themeable
it uses semantic markup, which is good practice, or as some say the Right Way
#3
The patch had DOS line endings and wouldn't apply.
I fully support this patch. +1
#4
I completely +1 this.
#5
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...
<?phpIndex: 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,
?>
#6
looks good to me. +1
#7
+1.
#8
and another +1
#9
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;
}
}
#10
Why do we need a drupal_get_help function at all? Why could the theme_help function not look like this:
<?phpfunction 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());.#11
Isn't it amazing that everyone nods '+1' without actually reviewing the code?
#12
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.
#13
Committed TDobes' latest version! Thanks.
#14
Well, I added the wrapper
drupal_get_help()to get more inline with the rest of the theming function likedrupal_get_title()anddrupal_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.
#15
Feel free to provide more patches. I'll review them when they hit the patch queue. Set the status back to 'patch' when there is a new patch to be reviewed.
#16
Moving out of the x.y.z queue into 6.x-dev. Technically, the theme system has been improving with each version, so it is probably safe to close this, but I'd rather have others decide.
#17
This patch has already been comitted.
#18
Automatically closed -- issue fixed for two weeks with no activity.