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

Reference: https://www.drupal.org/core/beta-changes
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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue tags: +Novice

Nice, thanks @LewisNyman!

star-szr’s picture

I would also add that we can use the extends trick, so the guts of the template would be something like the following:

{% extends "item-list.html.twig" %}
{% set attributes = attributes.addClass('search-results') %}

See field--text.html.twig.

jhodgdon’s picture

Issue tags: -Novice

Well. 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:

    $build['search_results'] = array(
      '#theme' => array('item_list__search_results__' . $plugin->getPluginId(), 'item_list__search_results'),
...
      '#list_type' => 'ol',
      '#attributes' => array(
        'class' => array(
          'search-results',
          $plugin->getPluginId() . '-results',
        ),
      ),

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.

star-szr’s picture

Thanks @jhodgdon you are correct. I'll take a look at this and try to get a patch together.

jhodgdon’s picture

I 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.

star-szr’s picture

Status: Active » Needs review
FileSize
2.78 KB

This 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.

jhodgdon’s picture

Looks 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.

star-szr’s picture

Issue tags: +Needs screenshots

Indeed.

deepak manhas’s picture

did a manual testing of search results.. here are the attached screenshots

search

search

search

search

search

search

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
25.27 KB
37.32 KB

Great, 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.

jhodgdon’s picture

OK let's get the patch file shown in the summary. ;)

star-szr’s picture

Issue tags: +Needs beta evaluation

Sorry I can't do it right now but this needs a beta eval. And thank you @jhodgdon @deepak manhas!

davidhernandez’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation
davidhernandez’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I 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.

davidhernandez’s picture

Status: Needs work » Reviewed & tested by the community

@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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I'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.

+++ b/core/includes/theme.inc
@@ -1774,7 +1774,7 @@ function drupal_common_theme() {
-      'variables' => array('items' => array(), 'title' => '', 'list_type' => 'ul', 'attributes' => array(), 'empty' => NULL),
+      'variables' => array('items' => array(), 'title' => '', 'list_type' => 'ul', 'attributes' => array(), 'empty' => NULL, 'context' => array()),

I agree that the theme system probably should support something generic but there is no harm in implementing this here.

  • alexpott committed 18cc89f on 8.0.x
    Issue #2495419 by Cottser: Move the 'search-results' class from the...

Status: Fixed » Closed (fixed)

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

star-szr’s picture