Refactor the Search module markup to be inline with our standards.

I'm working on this ticket right now.

Done with following:

1. Introduced BEM classes to all search result components
2. Update Classy styles
3. Checked Core styles
4. Tested regression

This ticket improves consistency and make search markup follow new drupal standarts and BEM.

No visual regression or changes as expected:

Before:

After:

Hm. So why is one of them changing to .search-result__title and the other to .search-result--snippet-info ? I don't understand the distinction.

jhodgdon, it's done to follow up BEM convention on naming elements.

.search-result - is a block
.search-result__title - is nested element of a block
.search-result__title--red - is a modified version of an nested element

Applying it to entire markup will make drupal 8 strongly consistent in terms of markup.

But isn't the snippet info also a nested element?

Yup!

+++ b/core/themes/bartik/css/components/search-results.css
@@ -20,9 +20,12 @@ ol.search-results {
+.search-result--snippet-info {
...
+[dir="rtl"] .search-result--snippet-info {

+++ b/core/themes/classy/templates/content/search-result.html.twig
@@ -57,15 +57,15 @@
+<div class="search-result__snippet-info">

These need to match up :) snippet-info is not a modifier I don't think.

You are right, jhodgdon. Good catch, I've fixed a patch.

OK, this looks more self-consistent. So, a question about the standards:

search-result__snippet-info is a div within search-result. And within this, we have p tags for a snippet and an info line. So should those be search-result__snippet-info__snippet and search-result__snippet-info__info? Right now they are search-result__snippet and search-result__info. I just don't know what the standards are.

By itself search-result__snippet-info__snippet or search-result__snippet-info__info is really descriptive, but in terms of understanding a component it's overtighten to a search block, where components are designed and should be independent entities. That's what I think, might be good point to asked BEM experts.

So really snippet-info is "snippet and other info div". Within that, there is the snippet and the info line. I just don't know what the standards are.

Search result is considered as block which is parent to elements. Where --info and --snippet are children as well it's not necessary when to be a children of --snippet-info element.

I.e, current state follows BEM standards and makes markup flexible enough to have:

search-result
--snippet-info
   --snippet
--custom-data
   --extra-data
   --info

for example. So we are keeping components flexible enough to be compatible with any search-result markup.

That makes sense to me. In which case, the latest patch should be fine, but should we make new screen shots just in case (manual test)? The "After" shot above is not relevant any more.

Here we go.

Great, thanks!

Fixed up beta eval in summary, which was there but not filled in.

https://www.drupal.org/node/1887918#sub-components seems like a good reference, for the record :)

+1 to RTBC.

Thanks for the reference to the standard and for the screenshots! Very helpful. I've embedded the latest screenshots in the summary. (It's always helpful to reviewers to do so.)

Note that, in general, we should also test CSS changes on RTL as well as LTR. (Reviewing the code carefully I think this is okay for RTL).

How I reviewed this:

1. +++ b/core/themes/bartik/css/components/search-results.css
   @@ -20,9 +20,12 @@ ol.search-results {
   -.search-results .search-snippet-info {
   ...
   +.search-result__snippet-info {
      padding-left: 0; /* LTR */
   ...
   -[dir="rtl"] .search-results .search-snippet-info {
   +[dir="rtl"] .search-result__snippet-info {
      padding-right: 0;
   
   +++ b/core/themes/classy/templates/content/search-result.html.twig
   @@ -57,15 +57,15 @@
   -<div class="search-snippet-info">
   +<div class="search-result__snippet-info">
   

   This is the first change, replacing the expectation of a certain markup nesting with explicit styling. This makes total sense to me even without being very familiar with the standard, and it definitely is in line with the reference: https://www.drupal.org/node/1887918#sub-components

   I grepped to confirm that (a) these were the only uses of search-snippet-info and (b) this wasn't inheriting any styling from search-results. The only direct styling for search-results is on ol.search-results and therefore not element to child items with the class.
    

2. +++ b/core/themes/bartik/css/components/search-results.css
   @@ -20,9 +20,12 @@ ol.search-results {
   +.search-result__title {
   +  font-weight: bold;
   +}
   
   +++ b/core/themes/classy/templates/content/search-result.html.twig
   @@ -57,15 +57,15 @@
   -<h3{{ title_attributes.addClass('title') }}>
   +<h3{{ title_attributes.addClass('search-result__title') }}>
   

   So this is the second change; instead of relying on .item-list .title from system.theme.css to make it bold, it adds a search-specific class to do that. I don't grok the standard that requires this entirely, but I'm trusting in @Cottser's signoff in addition to the RTBC. :)
    

3. +++ b/core/themes/classy/templates/content/search-result.html.twig
   @@ -57,15 +57,15 @@
   -    <p{{ content_attributes.addClass('search-snippet') }}>{{ snippet }}</p>
   +    <p{{ content_attributes.addClass('search-result__snippet') }}>{{ snippet }}</p>
   ...
   -    <p class="search-info">{{ info }}</p>
   +    <p class="search-result__info">{{ info }}</p>
   

   These are the third and fourth classes. I confirmed that there were no other uses of search-info or search-snippet outside of the classes added in this template.
    

I will say it's confusing that we have search-result__info, search-result__snippet, and search-result__snippet-info, all as different things. But that naming confusion wasn't introduced here, so maybe a followup?

This issue only changes user-facing strings, CSS, or markup, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x. Thanks!

Oh, an aside @Cottser: I just added the following URL alias for that page:
https://www.drupal.org/coding-standards/css/architecture

Thanks for the alias @xjm! Yes, change #2 threw me off but since it's Bartik it's to prevent any visual changes.

I'll create that follow up issue to discuss how we can improve the naming.

Follow-up is here: #2494665: Consider updating Classy's search result markup