Problem/Motivation

The markup of the templates within the Search module currently does not follow Drupal coding standards.

See search-result.html.twig in the Search module folder and in Classy.

Proposed resolution

We need to improve the code to follow BEM standards.

For example:

.search-results .search-snippet-info {

should be improved to be...

.search-results__snippet-info {

and so on.

We then need to go find any CSS that uses the markup (in the module, in Seven and Bartik) and make sure the selectors are changed so we do not cause any regressions.

We made some progress within a Bartik issue before we realised that this needs to be cleaned up in Core first, see the work here... #2486441: Clean up the "Search results" component in Bartik.

Remaining tasks

Write a patch,
Review the patch - code.
Visual review the patch to check for NO regressions.

User interface changes

None.

Before

After

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is a "be more in line with standards" change.
Issue priority Just normal.
Unfrozen changes Unfrozen because it only changes CSS and markup
Disruption Will affect any contributed themes deriving from Classy that include CSS for search result items and do not have their own template file. Should not affect anything else. But, it brings our search result template classes more in line with our standards, so it should be done anyway.
Files: 
CommentFileSizeAuthor
#17 classy-after.png455.7 KBcr0ss
#17 bartik-after.png418.66 KBcr0ss
#17 classy-before.png464.01 KBcr0ss
#17 bartik-before.png416.1 KBcr0ss
#11 search-introducing-BEM-2489394-2.patch1.53 KBcr0ss
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,827 pass(es). View
#5 search-introducing-BEM-2489394-1.patch1.53 KBcr0ss
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,367 pass(es). View
#2 search-introducing-BEM-2489394.patch1.48 KBcr0ss
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,342 pass(es). View

Comments

cr0ss’s picture

I'm working on this ticket right now.

cr0ss’s picture

Status: Active » Needs review
FileSize
1.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,342 pass(es). View

Done with following:

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

Status: Needs review » Needs work

The last submitted patch, 2: search-introducing-BEM-2489394.patch, failed testing.

Status: Needs work » Needs review
cr0ss’s picture

Issue summary: View changes
FileSize
410.61 KB
433.31 KB
1.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,367 pass(es). View

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

No visual regression or changes as expected:

Before:

Before

After:

After

cr0ss’s picture

Issue summary: View changes
jhodgdon’s picture

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.

cr0ss’s picture

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.

jhodgdon’s picture

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

Cottser’s picture

Status: Needs review » Needs work

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.

cr0ss’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,827 pass(es). View

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

jhodgdon’s picture

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.

cr0ss’s picture

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.

jhodgdon’s picture

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.

cr0ss’s picture

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.

jhodgdon’s picture

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.

cr0ss’s picture

Issue summary: View changes
FileSize
416.1 KB
464.01 KB
418.66 KB
455.7 KB

Here we go.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Great, thanks!

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

Cottser’s picture

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

+1 to RTBC.

  • xjm committed 07f92ed on 8.0.x
    Issue #2489394 by cr0ss, jhodgdon, Cottser: Refactor the Search module...
xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: -Novice

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!

xjm’s picture

Issue tags: +Needs followup
xjm’s picture

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

Cottser’s picture

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.

Cottser’s picture

Status: Fixed » Closed (fixed)

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