[drupal-devel] [task] improve theme system

[Dries]

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:   Dries
 Status:       patch

Isn't it amazing that everyone nods '+1' without actually reviewing the
code?




Dries



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());.