Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Search results are showing up with numbers.
This is apparently because the search result output is an OL list. However, I don't think we want the numbers displayed. They were not displayed in D7.
Screen shot attached.
Comment | File | Size | Author |
---|---|---|---|
#30 | 2115391-changed-file-name-30.patch | 1.2 KB | zaporylie |
#19 | 2115391-no-numbers-19.patch | 1.21 KB | jhodgdon |
#16 | nonumbers.png | 14.6 KB | jhodgdon |
#15 | interdiff-2115391-3-15.txt | 2.43 KB | lokapujya |
#15 | 2115391-15.patch | 1.24 KB | lokapujya |
Comments
Comment #1
jhodgdonActually, Stark has the same problem. Maybe we should solve this in a search.module CSS file?
Comment #2
BrightBoldComment #3
BrightBoldHere's a stab at adding a Search CSS file that solves this problem cross-theme. This looks good to me but if it's wrong, please know that this is only my third patch (core or contrib) ever!
Comment #4
lokapujyaPatch applies and It removes the numbering from Bartik and Stark results.
Comment #6
BrightBoldI looked at this (my first experience with unit testing!) and I can clearly reproduce that adding the
$variables['search_results']['#attached']['library']
intemplate_preprocess_search_results
is somehow messing up$variables['help']
, but I haven't yet figured out why. I will keep working on this.Comment #7
lokapujyaPreviously, search_results is empty when there are no results. But, now that we are adding the CSS file to the search_results array, search_results is not empty. Then, in the search-results.html.twig file, {% if search_results %} will always be true, and so will never get to the 'Your search yielded no results' part.
Comment #8
jhodgdonWe are planning to remove search-results.html.twig in this other issue:
#1926344: Consolidate search-result.html.twig and search-results.html.twig
so we should probably postpone this issue.
Comment #9
BrightBold@lokapujya — Thanks for the explanation. That makes sense.
Comment #10
jhodgdonThe other issue is long-since resolved, so this one can be un-postponed now. Markup is different.
Comment #11
lokapujya3: search-results-styling-2115391-3.patch queued for re-testing.
Comment #13
lokapujyaWhere do we attach the CSS now that template_preprocess_search_results() is gone?
Comment #14
jhodgdonRE #13, that is a very good question.
The search results page is built in \Drupal\search\Controller\SearchController::view(). It looks like in 8.x it is still possible to attach CSS file to a render array, so we should be able to put it in '#attached' as a library in $build['search_results'] or $build.
And looking at that previous patch... Libraries are not added with hook_library_info() any more, it looks like, but instead would go into a search.libraries.yml file. See change notice:
https://drupal.org/node/2201089
And it does look like making a library is the Drupal Way to attach CSS.
There is a CSS-only library YAML file example in core/modules/forum/forum.libraries.yml that should be useful to follow, although it looks like according to https://drupal.org/node/1887922 that we should put our CSS file into key "theme", not "component", since this is purely visual styling.
Hope that's enough to go on!
Comment #15
lokapujyaThis adds CSS based on the D8 Change Record mentioned in #13.
Comment #16
jhodgdonThis patch looked pretty good to me, but I tested it and it doesn't actually work.
I looked at some other uses of ['#attached']['library'] in Core and they look like this:
while this patch had:
When I changed the string at the end to array('search', 'drupal.search.results'), then it worked.
Also, there is extra whitespace on the line just before that.
So... Here is a new patch -- no interdiff, this is a small patch and both of the lines in SearchController changed (all else is the same). And here are some before/after screen shots:
Before (taken when I first filed this issue):
After (taken with different nodes in the DB with this new patch applied):
Comment #17
longwave@jhodgdon: Library references were changed very recently in #2203407: Replace #attached library array values with provider-namespaced strings, it looks like #15 is correct now to me?
Comment #18
jhodgdonlongwave: you're right. I forgot, I haven't done a pull on my repo recently. :(
OK, we need to skip the patch in #16 (I'll hide the file) and test the patch in #15 on a more recent install.
Comment #19
jhodgdonOK, now the patch in #15 works for me. It still has some trailing whitespace in the Controller section.
So here is a trivial redo of #15 without the extra spaces on that line. I will venture to go ahead and mark it RTBC since I have now verified that it works and longwave also looked at it.
Comment #22
longwave19: 2115391-no-numbers-19.patch queued for re-testing.
Comment #23
jhodgdonBack to RTBC after random test failure.
Comment #24
webchickThis looks like a good fix, but according to CSS file organization (for Drupal 8) (assuming I'm reading that correctly) it should be in a search.theme.css, methinks. (If I'm wrong about that, please don't kill me, and re-mark RTBC! :D)
Comment #25
jhodgdonI do not think that this applies when the CSS is included in a Library, which is what is done in this patch. We don't need that CSS file to be loaded on every page -- only on search results pages. Putting it in a library achieves that. I believe that if we put it into search.theme.css it would get loaded on every page.
This may not matter much, since there are only a few lines of CSS here... Maybe we can get someone involved in the theme system to comment?
Comment #26
star-szrBased on my reading of the CSS file organization docs I'd agree with @webchick that this should be added to search.theme.css.
Comment #27
LewisNymanThe reason for the *.theme.css files is so it's easy to remove all non-essential CSS. I think the changes here definitely fall under non-essential. With that in mind the correct name could be
search.theme.css
.Comment #28
jhodgdonThanks for clarifying! In that case, we should change the file name. The libraries thing is still necessary. Thanks!
Comment #29
zaporylieHi, I'm new but I want to try fix file name issue
Comment #30
zaporylieI have changed the file name.
Comment #31
jhodgdonThanks!
Confirmed this is exactly the same as the previous patch, with the file name change requested by webchick.
Also confirmed the patch works to remove the numbers on search results.
Let's get it in this time. :)
Comment #32
webchickAwesome, thanks!
Committed and pushed to 8.x.