Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
All classes are produced in PHP that makes it hard for a themer to manipulate these into whatever they need.
Proposed resolution
Copy the item-list.html.twig
class into Classy.
Rename it to item-list--search-results.html.twig
Add the class at the top of the Twig templates.
<div{{ attributes.addClass('search-results') }}>
Beta phase evaluation
Issue category | Task because it does not fix a bug or add a new feature, but it does improve the themer experience. |
---|---|
Issue priority | Not critical because it does not fix a critical problem. |
Unfrozen changes | Unfrozen because it only adds a template, which are not frozen. |
Disruption | There should be minimal disruption, if any. |
Comment | File | Size | Author |
---|---|---|---|
#6 | move_the-2495419-5.patch | 2.78 KB | star-szr |
Comments
Comment #1
star-szrNice, thanks @LewisNyman!
Comment #2
star-szrI would also add that we can use the extends trick, so the guts of the template would be something like the following:
See field--text.html.twig.
Comment #3
jhodgdonWell. So if you really want to get all of the classes out of the core modules and into Classy, you'll need to do more than this.
The code in SearchController does this:
So there are two classes being added: (a) 'search-results' and (b) the search plugin ID with -results appended, such as node_search-results or user_search-results for the two search plugins that are in Core.
Putting this other class into the Classy template is not going to be so straightforward so I am removing the Novice tag, assuming that you don't want this class to be added by the search controller either.
Comment #4
star-szrThanks @jhodgdon you are correct. I'll take a look at this and try to get a patch together.
Comment #5
jhodgdonI think the only way to make this work would be to add a custom, undocumented #property to that render array section, something like #search_plugin.
Comment #6
star-szrThis is where we kind of run into the fact that our theme system is really not component based, or the components we do have are not really fully thought out - they are just slightly evolved versions of what they've always been.
This is probably not the place for the discussion but this makes me think we should either allow arbitrary # keys for adding data in a render array, or have something like 'context' globally that can be populated (that is something that has been discussed before, with @Fabianx @Mark Carver et al.).
So this adds a 'context' variable to item_list to allow passing the additional data through.
Comment #7
jhodgdonLooks like it would probably work. Some before/after screen shots with Firebug or some other "inspect element", using Stark, Classy, and/or Bartik, would probably help.
Comment #8
star-szrIndeed.
Comment #9
deepak manhas CreditAttribution: deepak manhas commenteddid a manual testing of search results.. here are the attached screenshots
Comment #10
jhodgdonGreat, thanks very much for making the screen shots! So what we're losing here is that previously, the Search module made it so that although it was an OL list of search results, the numbers were not shown. With this patch, in Stark you get a numbered list. But that's consistent with Stark's other behavior of really having no classes (for instance, the Advanced search form is expanded).
So, the screen shots didn't include showing the classes in Classy, using "inspect element"... and I think they were done in Bartik and not Classy. I went ahead and made some for Classy with the classes shown, and I also verified it works correctly (classes match, looks same) for User search as well as Node search, although I didn't make more screen shots.
Assuming that we don't want classes or CSS at all in Stark, this seems right then.
Comment #11
jhodgdonOK let's get the patch file shown in the summary. ;)
Comment #12
star-szrSorry I can't do it right now but this needs a beta eval. And thank you @jhodgdon @deepak manhas!
Comment #13
davidhernandezComment #14
davidhernandezComment #15
alexpottI think this issue should move search.theme.css to classy as well since the CSS it has is no longer relevant to the search module with this patch.
Comment #16
davidhernandez@alexpott, that is being handled here, #2489578: move search.theme.css to classy , which is postponed on this one. This issue was created to sort out the class name/template part before proceeding with the css move.
Comment #17
alexpottI'm not sure that two issues are worth it but I'm not going to hold things up over that opinion.
Committed 18cc89f and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
I agree that the theme system probably should support something generic but there is no harm in implementing this here.
Comment #20
star-szrRelated issue for the #context things in #6 and #17: #2511548: Add a "context" array variable to all theme hooks and "#context" array property to all elements to provide optional contextual data