Problem/Motivation

Bartik's code needs to meet current Drupal coding standards.

Proposed resolution

This issue takes a specific section of Bartik's code, it acts as a "component" issue and improves that section of code with minimal impact on the rest of Bartik's codebase.

This issue aims to clean up and properly format the CSS and templates files without breaking Bartik visually.

The primary file for this issue is css/components/search.css file.

The search.css styles the following components in Drupal...
 

 
Within the code we have so many declarations where we declare both selectors to apply the same styles...

#search-block-form input.form-submit,
#search-form input.form-submit {

We should use one uniform class to declare both forms that have the same styles.
#search-form already has the class .search-form so we should add it to the search block markup too.

Along with this work we need to make sure we remove/fix redundant selectors and code. From using the audit tools in Chrome I have found the following selectors are not in the markup..

#block-search-form

Finally we need to ensure that when we refactor we use the Drupal coding standards - selectors following BEM, no IDs etc and fix the CSS lint errors listed here http://lewisnyman.co.uk/drupalcore-frontend-toolkit

See the META here #1342054: [META] Clean up templates and CSS for more details.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#68 clean_up_the_search-2486409-68.patch5.19 KBrachel_norfolk
#63 all-same-size.gif2.99 MBrachel_norfolk
#61 clean_up_the_search-2486409-61.patch5.2 KBhussainweb
#61 interdiff-59-61.txt1.98 KBhussainweb
#59 interdiff-58-59.txt923 bytesemma.maria
#59 clean_up_the_search-2486409-59.patch5.23 KBemma.maria
#59 Screen Shot 2015-08-13 at 14.22.59.png35.55 KBemma.maria
#59 Screen Shot 2015-08-13 at 14.22.50.png61.66 KBemma.maria
#59 safari2.png21.14 KBemma.maria
#59 safari.png26.05 KBemma.maria
#59 Screen Shot 2015-08-13 at 14.11.30.png32.32 KBemma.maria
#59 Screen Shot 2015-08-13 at 14.11.26.png21.59 KBemma.maria
#59 Screen Shot 2015-08-13 at 11.20.05.png33.48 KBemma.maria
#59 Screen Shot 2015-08-13 at 14.05.33.png24.31 KBemma.maria
#58 clean_up_the_search-2486409-58.patch5.05 KBemma.maria
#49 search-form__submit.png198.01 KBrachel_norfolk
#47 2015-08-04_1925.png114.75 KB_KurT_
#46 clean_up_the_search-2486409-46.patch5.63 KBLewisNyman
#46 interdiff.txt2.5 KBLewisNyman
#41 interdiff-31-41.patch922 bytesemma.maria
#41 bartik-search-styles-2486409-41.patch4.59 KBemma.maria
#41 Screen_Shot_2015-08-01_at_15_00_51.png181.64 KBemma.maria
#37 Screen Shot 2015-08-01 at 13.56.18.png45.77 KBemma.maria
#34 search-after.png121.54 KBManjit.Singh
#34 search-before.png120.87 KBManjit.Singh
#32 2015-07-30_1533.png44.83 KB_KurT_
#31 interdiff-2486409-22-31.txt485 bytes_KurT_
#31 bartik-search-styles-2486409-31.patch4.68 KB_KurT_
#30 Screen Shot 2015-07-27 at 20.00.38.png56.35 KBemma.maria
#25 after.png27.28 KBJohn Cook
#25 before.png26.9 KBJohn Cook
#23 bartik-search-styles-2486409-22.patch4.64 KBManjit.Singh
#19 interdiff.txt2.65 KBlauriii
#18 interdiff-2486409-8-16.txt3.64 KBandileco
#16 2486409_screenshot.jpg56.07 KBandileco
#16 bartik-search-styles-2486409-16.patch4.7 KBandileco
#12 8-before2.png31.2 KBByronNorris
#12 8-before1.png35.12 KBByronNorris
#12 8-after2.png31.01 KBByronNorris
#12 8-after1.png34.98 KBByronNorris
#8 interdiff-2486409-4-8.txt1.35 KB_KurT_
#8 bartik-search-styles-2486409-8.patch3.26 KB_KurT_
#4 bartik-search-styles-2486409-4.patch3.29 KBBrightBold
search-component.png128.64 KBemma.maria
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

emma.maria’s picture

BrightBold’s picture

Assigned: Unassigned » BrightBold
BrightBold’s picture

I have work that I've done on this but was unable to get it committed before I have to leave for the airport. So I'll get a patch posted later today, assuming wifi is working on my plane.

BrightBold’s picture

Assigned: BrightBold » Unassigned
FileSize
3.29 KB

This is incomplete but I wanted to post the portion I accomplished at the sprint today from the plane before my battery dies, so that if someone else wants to pick this up they can. This includes the following changes:

  • A new template suggestion and template for the search block form, with .search-form and .search-block-form classes added to mirror the search page's .search-form and .search-page-form classes.
  • A cleanup of search.css to replace all duplicate selectors with the unified .search-form. This included a cleanup of #block-search-form .form-item-search-block-form input to the simpler .search-form input#edit-keys but now I'm a little nervous that maybe that specificity was needed, although I couldn't see any reason for it when I removed it.

It does not include any work on coding standards, linting, BEM, etc.

emma.maria’s picture

Status: Active » Needs review
sarjeet.singh’s picture

Status: Needs review » Needs work
Issue tags: +SprintWeekend2015

SearchBlockForm is not rendering form template so there is no class in search block form . But SearchPageForm is rendering form template. This bug is in search module. We need to address this bug before fix this problem.

SearchBlockForm is rendering form template after below patch.

--- a/core/modules/search/src/Plugin/Block/SearchBlock.php
+++ b/core/modules/search/src/Plugin/Block/SearchBlock.php
@@ -34,7 +34,8 @@ protected function blockAccess(AccountInterface $account) {
* {@inheritdoc}
*/
public function build() {
- return \Drupal::formBuilder()->getForm('Drupal\search\Form\SearchBlockForm');
+ $form = \Drupal::formBuilder()->getForm('Drupal\search\Form\SearchBlockForm');
+ return array('form' => $form);
}

}

pakmanlh’s picture

I applied the #4 patch and sorry but I can't see what the comment #6 says, the new twig template is working without any necessity the change nothing.
Maybe you can add more information about that.

_KurT_’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
1.35 KB

Cleaned up previous patch.
Delete #block-search-form {padding-bottom: 7px;} line cause it was overridden by default.
Please tell me what else can i do here.

jgeryk’s picture

Status: Needs review » Reviewed & tested by the community

Applies cleanly on commit eb70a1c1cc13cc10dc73b2ead7b559095505fe65. Code appears compliant with Drupal CSS standards and has been cleaned up (IDs removed, selector redundancy fixed, etc).

emma.maria’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing, +Needs screenshots

Thank you @jgeryk for reviewing the code. As there are a lot of markup changes we need to carry out some visual regression testing to check we have not broken anything, putting this back to Needs Review.

ByronNorris’s picture

FileSize
34.98 KB
31.01 KB
35.12 KB
31.2 KB

Tested in Chrome Version 43.0.2357.132 (64-bit) on OSX Yosemite:

Prior to patch - standard view

After patch - standard view

Prior to patch - mobile view

After patch - mobile view

I cleared caches and inspected the form elements to verify the new markup was actually delivering the goods. I can do more screens if needed.

mgifford’s picture

Looks good to me. Should we be marking this as RTBC?

emma.maria’s picture

Assigned: Unassigned » emma.maria

@mgifford I can quickly check this today, it does very much look RTBC.

emma.maria’s picture

Assigned: emma.maria » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Needs screenshots

Wow great progress with the code so far. So happy we can use one class for two components. However I found a few things to improve in the code.

  1. +++ b/core/themes/bartik/css/components/search.css
    @@ -1,35 +1,27 @@
    +.search-block-form .content {
       margin-top: 0;
     }
    

    We do not need this declaration. display:inline is also active on this and that cancels out any margin applied to it.

  2.  

  3. The file should be named and only contain the main matching component class in the selectors. Therefore can search.css be changed to search-form.css. If any selectors starting with anything else exist they need to be moved out into a new component file of it's own. For eg. search-block-form.css.
  4.  

  5. The file does not have a correct file comment at the start of it.
    +++ b/core/themes/bartik/css/components/search.css
    @@ -1,35 +1,27 @@
     /* --------------- Search Form ---------------- */
     
    

    See for standards for this here.
    Look at existing CSS files within Bartik that have the correct file comments for the wording style Bartik is using.

  6.  

  7. +++ b/core/themes/bartik/css/components/search.css
    @@ -1,35 +1,27 @@
    +.search-form input[type="search"] {
       box-sizing: border-box;
       padding: 4px;
       -webkit-appearance: textfield;
     }
    

    We do not need the -webkit-appearance declaration here as normalize.css declares this also.

  8.  

  9. +++ b/core/themes/bartik/css/components/search.css
    @@ -1,35 +1,27 @@
    +.search-form input[type="search"] {
       box-sizing: border-box;
    

    Can we change any instances of input[type=... to use a class instead.

  10.  

  11. +++ b/core/themes/bartik/css/components/search.css
    @@ -1,35 +1,27 @@
    +[dir="rtl"] .search-form #edit-keys {
       float: right;
    

    Can we change IDs to use an available class instead.

andileco’s picture

OK, I think I made all the corrections in the attached patch. My changes ended up bumping the "Search help" link to the next line, but that makes more sense for me anyway. If not, I can easily downsize the search button so it goes back. Thanks @emma.maria for making this so easy!

I think I messed up my interdiff, so will try to recreate and upload.

andileco’s picture

Status: Needs work » Needs review
andileco’s picture

FileSize
3.64 KB

Still a little skeptical about this interdiff, but uploading it anyway.

lauriii’s picture

FileSize
2.65 KB

Interdiff between 8 and 16. Here's how you can do it by yourself: https://www.drupal.org/documentation/git/interdiff

lauriii’s picture

Status: Needs review » Needs work

Thanks for working on the issue. I was about to RTBC this but I think this is still worth fixing:

+++ b/core/themes/bartik/css/components/search-form.css
@@ -0,0 +1,38 @@
+.search-form .form-search {
+  box-sizing: border-box;
+  padding: 4px;
+  float: left; /* LTR */
+  font-size: 1em;
+  margin-right: 5px; /* LTR */
+}
...
+.search-form .form-submit {
+  margin-left: 0;
+  margin-right: 0;
+  height: 31px;
+  width: 34px;
+  padding: 0;
+  cursor: pointer;
+  text-indent: -9999px;
+  border-color: #e4e4e4 #d2d2d2 #b4b4b4;
+  background: #f0f0f0 url(../../../../misc/icons/505050/loupe.svg) no-repeat center;
+  overflow: hidden;
+}

This would become a lot easier to read if properties would be in alphabetical order

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh
Manjit.Singh’s picture

+++ b/core/themes/bartik/css/components/search-form.css
@@ -0,0 +1,38 @@
+  box-sizing: border-box;

We don't need this.. it is handled from normalize.css file




+++ b/core/themes/bartik/css/components/search-form.css
@@ -0,0 +1,38 @@
+.search-form .form-search::-webkit-search-decoration {
+  display: none;
+}

Dont know where we are using this CSS :(

+++ b/core/themes/bartik/css/components/search-form.css
@@ -0,0 +1,38 @@
+.search-form .form-submit {
...
+  border-color: #e4e4e4 #d2d2d2 #b4b4b4;
...
+}

We can remove this css also, This is handled with buttons.css file.

@lauriii can you please review the changes that i have suggested.

Manjit.Singh’s picture

updating patch according to #20 and #22 :)

Manjit.Singh’s picture

Assigned: Manjit.Singh » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs screenshots
John Cook’s picture

FileSize
26.9 KB
27.28 KB

Before patch:
Before patch
After patch:
After patch

The "Search help" link has moved from under the button to under the text box.

lauriii’s picture

Status: Needs review » Needs work

Seems to need some more work

emma.maria’s picture

Status: Needs work » Needs review

I actually really like the search help being underneath the search field, in hindsight the before now looks like a visual bug. I will check the code out, to see what happened.

andileco’s picture

I believe the reason it appeared there before was just that the search button was shorter than the text input, so there was room for the help text to float up next to the button.

emma.maria’s picture

Ah yes that's it. I would love to sort out the height and shape of the search submit buttons ASAP, I will raise as a follow up. The squircles with their mis-matched heights with the inputs give me nightmares :)

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
56.35 KB
  1. +++ b/core/themes/bartik/css/components/search-form.css
    @@ -0,0 +1,36 @@
    +/**
    + * @file
    + * The visual styles for Bartik's search form(s).
    + */
    +.search-form .form-search {
    

    We need to add a blank line after the file comment.

  2.  

  3. The search submit buttons in my browser look broken also and also different to how #25 sees it. I think we have removed to much in #23's patch.
_KurT_’s picture

Status: Needs work » Needs review
FileSize
4.68 KB
485 bytes

Seems like i fixed search submit button height.

_KurT_’s picture

FileSize
44.83 KB

Also i would like to suggest to move loupe icon on 1px right. Yes i know that now it is right on center url(../../../../misc/icons/505050/loupe.svg) no-repeat center, but IMHO if you'll take a look more attentive, you'll see that for eyes (not numbers in code) it looks better when bigger part of image is on center. Hope i explained it correctly.

emma.maria’s picture

@_KurT_ Please feel free to raise a separate issue about the placement of the Search icon :)

Manjit.Singh’s picture

FileSize
120.87 KB
121.54 KB

@_KurT_ @emma Please find the screenshots.

Is there any need to changes "Advance search" to an icon. Or we are changing whole search buttons into icons ?

Before
alt

After
alt

_KurT_’s picture

I guess we need leave text on button in advanced search. If we are, i found couple ways to do it, first is to use not() selector:
.search-form .form-wrapper:not(.search-advanced) .form-submit
instead of
.search-form .form-submit
and second is to add new class to basic search. Advanced search already have its unique class - search-advanced, maybe basic search need to have such too, e.g. search-basic. As i said, in first way we can do it only by css, but it will make css a bit more complicated and less readable.

emma.maria’s picture

Assigned: Unassigned » emma.maria
emma.maria’s picture

Issue summary: View changes
FileSize
45.77 KB

@Manjit.Singh the search icons are only for the search form fields. I cannot reproduce seeing buttons with "Search" printed in them like your Before screenshot in #34.

Search form screenshot at Head right now.

 

 
@everyone we are not trying to improve the design of the search in this issue. We are cleaning up the CSS only and not adding conscious design changes. Please raise follow ups for any design changes you would like to suggest.

The fix for the search submit buttons being too tall within #31 is incorrect. The issue is that the search submit buttons became too tall from the work in this issue. The input text boxes are the correct height, we shouldn't increase these also.

Setting to needs work.

emma.maria’s picture

@Manjit.Singh the search icons are only for the search form fields. I cannot reproduce seeing buttons with "Search" printed in them like your Before screenshot in #34.

Search form screenshot at Head right now.

 

 
@everyone we are not trying to improve the design of the search in this issue. We are cleaning up the CSS only and not adding conscious design changes. Please raise follow ups for any design changes you would like to suggest.

The fix for the search submit buttons being too tall within #31 is incorrect. The issue is that the search submit buttons became too tall from the work in this issue. The input text boxes are the correct height, we shouldn't increase these also.

Setting to needs work.

Manjit.Singh’s picture

@emma.maria check after screenshot in #34.

In Advance search section, Before it was an Advance search button, But now there would be an icon, So I was just clarifying that Is it ok ?

emma.maria’s picture

@Manjit.Singh oh no! I didn't see that! I thought you was talking about what you saw in your first screenshot where I can see "Search" text in the buttons.

OK so we need to apply a class to the search submit buttons instead of using the generic .form-submit class. So we can target what we want.

I have created a patch to clean up the styles. Every font input and submit will be the same height wherever it is posted, I found an issue where a font size applied to the sidebar made the round submit button look crappy as it became inconsistent with the styles for search forms (it applied a different value of border radius, etc).

emma.maria’s picture

Screenshot.
 

 

Also patch and interdiff attached.

emma.maria’s picture

We still need to replace the .search-form .form-submit with a reusable class that we can apply to buttons that we want to have the search icon styles.

Status: Needs review » Needs work

The last submitted patch, 41: interdiff-31-41.patch, failed testing.

emma.maria’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Needs work

According to #42 NW

LewisNyman’s picture

Ok, this might not be the best way to do it but this what we should aim for. I had to add .button to the selectors because form.css contains some very strong selectors for input.form-submit

_KurT_’s picture

Issue summary: View changes
FileSize
114.75 KB

@emma.maria you said

Every font input and submit will be the same height wherever it is posted

but it is not. After applying #41 patch i have different height of input and submit
input height in chrome
I have chrome 44.0.2403.125 windows 7.

Input height is different in different browsers, chrome and safari. Guess you are checking in safari, so we have some misunderstanding, I've checked in chrome. Height of button is the same, because it setted directly by height rule. Height of input is not setted directly, you can see height rule is a bit lighter than others, if it is not - it came from its line-height, line-height is not setted directly too, line-height is relative to font-size, font-size is settted not directly too, it is always in relative value, so we should look to default browser font-size. I don't know why inputs have different height, because afaik chrome and safari have same default font-size (16px).

rachel_norfolk’s picture

Working on this at Drupalaton...

As I'm seeing things right now, we are selecting the submit button correctly in the Search Block but not the submit button in the Advanced Search page form.

Looking into why and what to do about it.

rachel_norfolk’s picture

Issue summary: View changes
FileSize
198.01 KB

Might be due to not adding a class

class missing screenshot

emma.maria’s picture

Issue tags: +Needs reroll
emma.maria’s picture

Status: Needs review » Needs work
emma.maria’s picture

Issue tags: -Needs reroll

OK it doesn't need a reroll.

But there are two declarations of the button.css file within bartik.libraries.yml which was introduced in patch #46. This has nothing to do with this issue and got introduced in a reroll due to this being committed #2542604: Clean up the "buttons" component in Bartik.

Please remove :)

+++ b/core/themes/bartik/bartik.libraries.yml
@@ -8,6 +8,7 @@ global-styling:
       css/components/breadcrumb.css: {}
+      css/components/buttons.css: {}
       css/components/captions.css: {}
rachel_norfolk’s picture

will sort...

rachel_norfolk’s picture

Issue summary: View changes
rachel_norfolk’s picture

Issue summary: View changes

The last submitted patch, 46: clean_up_the_search-2486409-46.patch, failed testing.

emma.maria’s picture

Status: Needs work » Needs review
FileSize
5.05 KB

Rerolled plus removed the extra declaration of buttons.css that snuck into the libraries file which is unrelated to this issue.

emma.maria’s picture

#49 I had to add another hook_form_alter to add the BEM class to the other submit button.

@LewisNyman the classes are now added to both submit buttons, can you please check over the code I added to your solution?

Regarding #47
I cannot replicate the heights being different. Both forms share the same class and I develop in Chrome and further checked Firefox and Safari for inconsistencies and there are none. I can't see how there would be.

Screenshots

Chrome

Search form block

Search form

Firefox

Search form block

Search form

Safari

Search form block

Search form

Both forms in one screenshot

Advanced search stays the same

star-szr’s picture

Status: Needs review » Needs work

PHP/Twig review :)

  1. +++ b/core/themes/bartik/bartik.theme
    @@ -110,6 +110,29 @@ function bartik_preprocess_menu(&$variables) {
    +  if($form_id == 'search_block_form') {
    ...
    +  if($form_id == 'search_form') {
    

    Minor: Needs a space between if and (.

    Also maybe this could be a switch/case or in_array()?

    if (in_array($form_id, ['search_block_form', 'search_form'])) {

  2. +++ b/core/themes/bartik/bartik.theme
    @@ -110,6 +110,29 @@ function bartik_preprocess_menu(&$variables) {
    +    $form['actions']['submit']['#attributes'] = new Attribute();
    

    Not totally sure about the new Attribute here, that may override things earlier in the stack. Did you try:

    $form['actions']['submit']['#attributes']['class'][] = 'search-form__submit';

  3. +++ b/core/themes/bartik/templates/form--search-block-form.html.twig
    @@ -0,0 +1,17 @@
    + * Available variables
    

    This needs to end in a colon per https://www.drupal.org/node/1354#lists.

  4. +++ b/core/themes/bartik/templates/form--search-block-form.html.twig
    @@ -0,0 +1,17 @@
    + *
    + * @ingroup themeable
    

    Remove @ingroup themeable, that should only be on core templates.

  5. +++ b/core/themes/bartik/templates/form--search-block-form.html.twig
    @@ -0,0 +1,17 @@
    +<form{{ attributes.addClass('search-form','search-block-form') }}>
    

    Minor: Should be a space after the comma.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
5.2 KB

Fixed for comments in #60. I did a different kind of implementation for point 2. Let me know what you think.

emma.maria’s picture

Issue tags: +Twig
rachel_norfolk’s picture

FileSize
2.99 MB

They're definitely all the same height - yay!!!

Checked Firefox, Chrome and Safari.

https://www.drupal.org/files/issues/all-same-size.gif

emma.maria’s picture

Thanks @rachel_norfolk :)

Can anyone do a follow up PHP/Twig review of the improvements in #61?

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Twig/PHP looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll.

rachel_norfolk’s picture

I'm on it...

rachel_norfolk’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Ah yes - was just that the functions for adding template hints was confusing the patch given so much has been removed by moving logo etc into blocks.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Still looks good! Tested manually on Bartik home page and no visual changes! :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 997da33 and pushed to 8.0.x. Thanks!

Fixed incorrect file modes on commit.

diff --git a/core/themes/bartik/bartik.theme b/core/themes/bartik/bartik.theme
old mode 100755
new mode 100644
diff --git a/core/themes/bartik/templates/form--search-block-form.html.twig b/core/themes/bartik/templates/form--search-block-form.html.twig
old mode 100755
new mode 100644

  • alexpott committed 997da33 on 8.0.x
    Issue #2486409 by emma.maria, _KurT_, rachel_norfolk, Manjit.Singh,...

Status: Fixed » Needs work

The last submitted patch, 68: clean_up_the_search-2486409-68.patch, failed testing.

emma.maria’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

Jeff Burnz’s picture

Version: 8.0.x-dev » 9.x-dev

I saw someone posting the php from this patch on stackexchange as an example - I am wondering why this might want to do a new Attributes if (!isset($form[$key]['submit']['#attributes']))..., I mean, under what circumstances would #attributes not be set?

I use a rather gnarly switch statement for form alters, I've been doing this for about a year in D8, no issues:

  switch ($form_id) {
    case 'search_block_form':
      $form['actions']['submit']['#attributes']['class'][] = 'search-form__submit';
      .. etc
      break;

    case 'search_form':
      $form['basic']['submit']['#attributes']['class'][] = 'search-form__submit';
      ...etc
  }

That isset() check looks redundant to me, unless I am missing something?

http://drupal.stackexchange.com/questions/206846/twig-template-for-searc...

Jeff Burnz’s picture

Version: 9.x-dev » 8.0.0

No idea why that changed...