[drupal-devel] [feature] New themable function theme_search_form to unify search form and put logic at the correct level
Issue status update for http://drupal.org/node/27843 Post a follow up: http://drupal.org/project/comments/add/27843 Project: Drupal Version: cvs Component: search.module Category: feature requests Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: robertDouglass Status: patch Attachment: http://drupal.org/files/issues/make_search_themable.txt (3.99 KB) The search form should be themable, but it shouldn't be left to the theme('page') function to do this, as is the current implementation. This patch adds a theme_search_form function to theme.inc (not search module because that would cause a dependency) and removes the then unneeded variables from the phptemplate.engine. Pushbutton and Bluemarine are updated to take advantage of the new function. In the current implementation, several variables are created needlessly in phptemplate.engine (like the text for the button) and then pushed to the template. This seems messy and this patch fixes that. Remaining questions: should the function accept any parameters? I think not, but other opinions are welcome. robertDouglass
Issue status update for http://drupal.org/node/27843 Post a follow up: http://drupal.org/project/comments/add/27843 Project: Drupal Version: cvs Component: search.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: Steven Status: patch This patch collides with another patch where search_form() is turned into theme_search_form(). But search_form() is more complicated (especially in light of my upcoming search changes [1]). So there is a compact and a full-blown style to accomodate. Still, typically search forms in the theme are designed specifically to fit in, with a custom search button and whatnot. They are a very typical theme feature: why not keep it simple and inside page.tpl.php? The dependency problem is also hairy... theme_search_form() does not belong in theme.inc at all IMO, especially because it will become quite logic-heavy. Possible solutions: - Modify this patch to use theme_search_form_compact(), while theme_search_form() is reserved for the full search form. - Keep the built-in theme search in the themes and apply the other patch. The only themable function would be for the full search form. [1] http://www.acko.net/dumpx/searcholeet.png Steven Previous comments: ------------------------------------------------------------------------ Sat, 30 Jul 2005 12:03:02 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/make_search_themable.txt (3.99 KB) The search form should be themable, but it shouldn't be left to the theme('page') function to do this, as is the current implementation. This patch adds a theme_search_form function to theme.inc (not search module because that would cause a dependency) and removes the then unneeded variables from the phptemplate.engine. Pushbutton and Bluemarine are updated to take advantage of the new function. In the current implementation, several variables are created needlessly in phptemplate.engine (like the text for the button) and then pushed to the template. This seems messy and this patch fixes that. Remaining questions: should the function accept any parameters? I think not, but other opinions are welcome.
Issue status update for http://drupal.org/node/27843 Post a follow up: http://drupal.org/project/comments/add/27843 Project: Drupal Version: cvs Component: search.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: Jose A Reyero Status: patch (code needs review) I think this is not needed anymore, now we have that multiple regions for blocks and can just place a search block anywhere in the page. In fact, that search forms in themes should be replaced by standard blocks. Jose A Reyero Previous comments: ------------------------------------------------------------------------ Sat, 30 Jul 2005 12:03:02 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/make_search_themable.txt (3.99 KB) The search form should be themable, but it shouldn't be left to the theme('page') function to do this, as is the current implementation. This patch adds a theme_search_form function to theme.inc (not search module because that would cause a dependency) and removes the then unneeded variables from the phptemplate.engine. Pushbutton and Bluemarine are updated to take advantage of the new function. In the current implementation, several variables are created needlessly in phptemplate.engine (like the text for the button) and then pushed to the template. This seems messy and this patch fixes that. Remaining questions: should the function accept any parameters? I think not, but other opinions are welcome. ------------------------------------------------------------------------ Sat, 30 Jul 2005 12:37:13 +0000 : Steven This patch collides with another patch where search_form() is turned into theme_search_form(). But search_form() is more complicated (especially in light of my upcoming search changes [1]). So there is a compact and a full-blown style to accomodate. Still, typically search forms in the theme are designed specifically to fit in, with a custom search button and whatnot. They are a very typical theme feature: why not keep it simple and inside page.tpl.php? The dependency problem is also hairy... theme_search_form() does not belong in theme.inc at all IMO, especially because it will become quite logic-heavy. Possible solutions: - Modify this patch to use theme_search_form_compact(), while theme_search_form() is reserved for the full search form. - Keep the built-in theme search in the themes and apply the other patch. The only themable function would be for the full search form. [1] http://www.acko.net/dumpx/searcholeet.png
participants (3)
-
Jose A Reyero -
robertDouglass
-
Steven