move search.theme.css to classy

moving to classy :)

Nice! This looks simple, now we just need some screenshots in Classy.

Adding Novice tag to the issue for screenshots.

Needs work for needs tag

Are these screenshots all that is needed here then?

I think the screenshots we need are to show that the output from Classy is the same before and after the patch. As long as Stark works it doesn't need to look pretty, but moving the CSS here shouldn't break anything coming out of Classy.

before and after screenshots are same in classy :)

Based on the discussion at #2489460: [Meta] Move module.theme.css files to Classy or Seven, at this point library names in Classy should match what is in the core module. So, search-results should be search.results.

I processed the remarks mentioned by @jstoller in #10 in this patch.

If the test passes, we need some new screenshots. Thanks for working on this pjbaert!

3rd and 4th screenshots have some issue at right side while capturing due to scrollbar ;)

This change does not make sense to me.

The code that makes the ol.search-results is in the SearchController:

    $build['search_results'] = array(
      '#theme' => array('item_list__search_results__' . $plugin->getPluginId(), 'item_list__search_results'),
      '#items' => $results,
      '#empty' => array(
        '#markup' => '<h3>' . $this->t('Your search yielded no results.') . '</h3>',
      ),
      '#list_type' => 'ol',
      '#attributes' => array(
        'class' => array(
          'search-results',
          $plugin->getPluginId() . '-results',
        ),
      ),

It is *not* in the Classy theme. So the CSS should remain here in the Search module.

Also it definitely is not part of the individual search result template, it is the overall list that contains the individual search results. So I think this is a Works as Designed issue. You'd need to totally rework this code in order to make it part of Classy, and we got rid of the search results template in 8 because it wasn't really doing anything and made it a classed OL list instead on purpose.

Based on the original Consensus Banana in Austin, all markup and CSS that isn't absolutely required for the bare minimum functionality of a module should be moved to Classy. Later this was revised, compromising with Core branch maintainers, so that code dealing with purely administrative functions would stay in the core modules, but all public-facing markup would still move to Classy. This was reaffirmed at the DrupalCon sprint in LA.

There are a number of grey areas we're finding, but it seems like search results unambiguously fall into the public-facing side of this split. Technically the search module shouldn't be making any markup decisions here. I don't know if that is possible at this point, but we should try to get as close to the ideal as we can. If we can move the CSS to Classy then we should do so, even if the classes it is styling are coming from the SearchController.

OK. Well we'll need a completely different patch then.

The search module controller is not really making a markup decision. It is only saying "This is an ordered list of items of type search results" in its render array.

Then the CSS is only saying "yeah it's an ordered list, but normally you wouldn't want the numbers on it".

I don't see how this can/should be moved to Classy, but am happy to review patches that do it.

It looks like this class was missed during #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates. - I've opened #2495419: Move the 'search-results' class from the render array and into a Classy template so we can move the classy into Classy before we move the CSS here.

Time to make this Active again, as the classes are now in Classy rather than the render array. So I think all that remains here is to move the CSS file?

At a glance it looks like maybe a reroll of #11 would do the trick.

rerolled the patch in #11

Looks reasonable, thanks @saki007ster! Needs screenshots/manual testing now.

@cottser I am searching text with keywords "test content". but there is no result with that keywords. After that i am searching with advance search and but previous keywords not going override with the new keywords i enter in advance search. Is that issue or its part of functionality.

adding screencast !!

What we need to test manually:

a) Create some content and run Cron so that you have something to search.

b) Search for keywords in your content.

==> Then for both Stark and Classy themes (and also for Bartik maybe, just to be sure?), we would want to verify that the markup and look of the search results page are as expected (before and after screen shots of the output and the markup would be helpful).

But... Before we do that... I'm looking at this patch, and it doesn't actually move the search.theme.css file to the Classy theme. So we need to fix that first.

+++ b/core/themes/classy/classy.libraries.yml
@@ -15,3 +15,9 @@ drupal.comment.threaded:
diff --git a/core/modules/search/css/search.theme.css b/core/themes/classy/css/search/search.theme.css
similarity index 100%
rename from core/modules/search/css/search.theme.css
rename to core/themes/classy/css/search/search.theme.css

This should move the file, but it's subtle if you're just looking at the patch file.

Doh! I missed that. Sorry!

I was going to test this and make screen shots today but the patch does not apply.

Rerolled after #2491259: move search.admin.css into seven. Thanks @jhodgdon!

+++ b/core/themes/classy/templates/content/search-result.html.twig
@@ -56,6 +56,7 @@
+{{ attach_library('classy/search.results') }}

Hm. The CSS file is providing CSS for the ol.search-results list as a whole. This is attaching it in the template for each individual search result... but I guess that is OK, because we don't need the list style if it's an empty list I guess. And I hope/suspect Drupal is smart enough only to attach the CSS file once per page.

So, that aside, let's see about screen shots. To make these, I used Standard profile, added a node to the site, ran cron, and took screen shots of the search results page with Classy and Bartik.

They look identical to me. Seems good to go. Thanks!

Ace, thanks for the work here.

The theme layer is not yet frozen. Committed 42ff057 and pushed to 8.0.x. Thanks!

For some reason this breaks my sites CSS/JS optimization. I don't have any clue why it happens.

Nothing to worry about. Bug was something that has nothing to do with this issue