When search results pages are rendered in the current 7.x-dev version of the Search module, the search results markup is passed to the theme function theme_search_results_listing() which prefixes an <h2 class="title"> tag with a title (passed by function argument) to the markup.

However, in recent revisions this title argument will never have a value (leading to an empty h2 tag which causes inconsistent styling) as the titles for the search results are now templated by search-results.tpl.php, and without a title being appended then the function has no real purpose and should be removed. Furthermore, the actual titles in search-results.tpl.php should then have class="title" as an attribute as opposed to the empty tag.

Thoughts/suggestions?

CommentFileSizeAuthor
#13 search-results_styling-838014-13.patch1.93 KBcomrade.salazar
#9 search-results_styling-838014-9.patch2.28 KBcomrade.salazar
#6 search-results.png20.31 KBdawehner
#6 search-results-after.png10.62 KBdawehner
#6 search-results-styling_4_0.patch2.21 KBdawehner
#3 search-results-styling.patch2.21 KBcomrade.salazar
#1 search-styling-838014-1.patch2.27 KBcomrade.salazar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

comrade.salazar’s picture

comrade.salazar CreditAttribution: comrade.salazar commented
FileSize
search-styling-838014-1.patch2.27 KB

Here is the patch, it didn't seem to attach the first time.

Status: Needs review » Needs work

The last submitted patch, search-results-styling.patch, failed testing.

comrade.salazar’s picture

comrade.salazar CreditAttribution: comrade.salazar commented
FileSize
search-results-styling.patch2.21 KB

Okay, rookie mistake, here's the patch with unix-style endings:

comrade.salazar’s picture

comrade.salazar CreditAttribution: comrade.salazar commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, search-results-styling.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
search-results-styling_4_0.patch2.21 KB
search-results-after.png10.62 KB
search-results.png20.31 KB

Here is a screenshot which shows the behavior.

Here is a rerolled patch

Status: Needs review » Needs work

The last submitted patch, search-results-styling_4_0.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Ummm. I'm not sure that removing theme('search_results_listing') is the right answer to this problem.

And your patch is also adding a class to the H2, which is probably an OK idea, but I think the class should be more specific than "title". Maybe class="search-results-title"?

comrade.salazar’s picture

comrade.salazar CreditAttribution: comrade.salazar commented
Status: Needs work » Needs review
FileSize
search-results_styling-838014-9.patch2.28 KB

My reasoning for the removal of theme_search_results_listing() is once the code adding the empty <h2> tag is removed (it seems from the revision history that the reason the inclusion of the title was moved from the function to the template file was for better customizability), then the only purpose the function would have would be to simply return the template file's markup verbatim. Wouldn't it be better if this now-redundant theme function were simply removed and the theming of the search results overall would be solely through overriding search-results.tpl.php?

I agree with the class="search-results-title" idea though, I was just going with the class="title" that the empty tag had (which in addition is already used by <h1 class="title">Search<h1>, the actual heading of the page).

The attached patch fixes the incorrect paths in the previous patches, and changes the class information of the <h2> tags to search-results-title.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

OK on the philosophy/reasoning...

Hmmm...

One more detail. I'm wondering whether the H2 for the search results title is really equivalent to the H2 saying there were no search results. Maybe they should have different classes?

And another detail: search_results_listing also needs to be removed from search_theme() if we are getting rid of it.

I was also bothered by the fact that search_view() is returning already-themed search results, but that is a separate issue. We really ought to save the theming until the last minute in the Search module... Just filed:
#839524: Search results are themed too early

comrade.salazar’s picture

comrade.salazar CreditAttribution: comrade.salazar commented

Thanks for catching that; sorry for the oversight with the search_theme() function.

I'm still wondering why we'd have to be so specific with the class name. The context and location in which both would be displayed in is similar enough, and if someone wanted to style the two headings differently this could be done by implementing search-results.tpl.php.

And with the specificity of "search-results-title" vs "title", note that the title for each individual search result is given class="title" already, and that many themes give the main title of each page/node class="title" as well. Making the class="title" for these h2 headers leads to the overuse of this class name though gives it proper context, using class="search-results-title" makes the headers unnecessarily distinct form other titles.

Because of this, I'm starting to think that the search result headers should just have no pre-defined classes (the current way, starting to think this is best). If the theme wants them, the theme can style the h2 headers in the context of the search results or add its own class via search-results.tpl.php.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

OK. I'm not all that concerned... As you say, it is in a template, so a theme can easily change it. Fine either way.

comrade.salazar’s picture

comrade.salazar CreditAttribution: comrade.salazar commented
Status: Needs work » Needs review
FileSize
search-results_styling-838014-13.patch1.93 KB

Okay, this should be the final version of the patch. search.module and search.pages.inc reflect the removal of theme_search_results_listing(), and search-results.tpl.php stays as it currently is in CVS (without class="title" headings).

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Incidentally, this theme('search_results_listing') was added on this issue:
#372471: Kill box.tpl.php / theme_box()
And then the title was removed sometime later (cannot seem to locate that issue).

If this patch is accepted, we need to update the information on the theme guide here:
http://drupal.org/update/theme/6/7#box

Anyway. I think this patch is fine. We don't really need this theme function in the search module, as far as I can see, since both the results and the form are already themeable, and they are put into a page which you could also theme. The search results listing theme function was a simple wrapper on the already-themeable results section anyway -- it's not necessary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

This issue would help to do http://drupal.org/node/839524

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Right. I just filed that other issue yesterday. It needs to wait until this one gets in.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Alright then. Committed to CVS HEAD. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I updated the theme page too.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.