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
Comment | File | Size | Author |
---|---|---|---|
#68 | clean_up_the_search-2486409-68.patch | 5.19 KB | rachel_norfolk |
#63 | all-same-size.gif | 2.99 MB | rachel_norfolk |
#61 | clean_up_the_search-2486409-61.patch | 5.2 KB | hussainweb |
#61 | interdiff-59-61.txt | 1.98 KB | hussainweb |
#59 | interdiff-58-59.txt | 923 bytes | emma.maria |
Comments
Comment #1
emma.mariaComment #2
BrightBoldComment #3
BrightBoldI 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.
Comment #4
BrightBoldThis 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:
.search-form
and.search-block-form
classes added to mirror the search page's.search-form
and.search-page-form
classes..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.
Comment #5
emma.mariaComment #6
sarjeet.singh CreditAttribution: sarjeet.singh as a volunteer commentedSearchBlockForm 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);
}
}
Comment #7
pakmanlhI 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.
Comment #8
_KurT_ CreditAttribution: _KurT_ at FFW commentedCleaned 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.
Comment #9
jgeryk CreditAttribution: jgeryk commentedApplies cleanly on commit eb70a1c1cc13cc10dc73b2ead7b559095505fe65. Code appears compliant with Drupal CSS standards and has been cleaned up (IDs removed, selector redundancy fixed, etc).
Comment #11
emma.mariaThank 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.
Comment #12
ByronNorris CreditAttribution: ByronNorris at Acquia commentedTested in Chrome Version 43.0.2357.132 (64-bit) on OSX Yosemite:
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.
Comment #13
mgiffordLooks good to me. Should we be marking this as RTBC?
Comment #14
emma.maria@mgifford I can quickly check this today, it does very much look RTBC.
Comment #15
emma.mariaWow 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.
We do not need this declaration. display:inline is also active on this and that cancels out any margin applied to it.
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.
We do not need the -webkit-appearance declaration here as normalize.css declares this also.
Can we change any instances of input[type=... to use a class instead.
Can we change IDs to use an available class instead.
Comment #16
andileco CreditAttribution: andileco as a volunteer commentedOK, 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.
Comment #17
andileco CreditAttribution: andileco as a volunteer commentedComment #18
andileco CreditAttribution: andileco as a volunteer commentedStill a little skeptical about this interdiff, but uploading it anyway.
Comment #19
lauriiiInterdiff between 8 and 16. Here's how you can do it by yourself: https://www.drupal.org/documentation/git/interdiff
Comment #20
lauriiiThanks for working on the issue. I was about to RTBC this but I think this is still worth fixing:
This would become a lot easier to read if properties would be in alphabetical order
Comment #21
Manjit.SinghComment #22
Manjit.SinghWe don't need this.. it is handled from normalize.css file
Dont know where we are using this CSS :(
We can remove this css also, This is handled with buttons.css file.
@lauriii can you please review the changes that i have suggested.
Comment #23
Manjit.Singhupdating patch according to #20 and #22 :)
Comment #24
Manjit.SinghComment #25
John Cook CreditAttribution: John Cook commentedBefore patch:
After patch:
The "Search help" link has moved from under the button to under the text box.
Comment #26
lauriiiSeems to need some more work
Comment #27
emma.mariaI 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.
Comment #28
andileco CreditAttribution: andileco as a volunteer commentedI 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.
Comment #29
emma.mariaAh 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 :)
Comment #30
emma.mariaWe need to add a blank line after the file comment.
Comment #31
_KurT_ CreditAttribution: _KurT_ at FFW commentedSeems like i fixed search submit button height.
Comment #32
_KurT_ CreditAttribution: _KurT_ at FFW commentedAlso 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.Comment #33
emma.maria@_KurT_ Please feel free to raise a separate issue about the placement of the Search icon :)
Comment #34
Manjit.Singh@_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
After
Comment #35
_KurT_ CreditAttribution: _KurT_ at FFW commentedI 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.Comment #36
emma.mariaComment #37
emma.maria@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.
Comment #38
emma.maria@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.
Comment #39
Manjit.Singh@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 ?
Comment #40
emma.maria@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).
Comment #41
emma.mariaScreenshot.
Also patch and interdiff attached.
Comment #42
emma.mariaWe 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.Comment #44
emma.mariaComment #45
lauriiiAccording to #42 NW
Comment #46
LewisNyman CreditAttribution: LewisNyman at Wunder commentedOk, 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 forinput.form-submit
Comment #47
_KurT_ CreditAttribution: _KurT_ at FFW commented@emma.maria you said
but it is not. After applying #41 patch i have different height of input and submit
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).
Comment #48
rachel_norfolkWorking 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.
Comment #49
rachel_norfolkMight be due to not adding a class
Comment #50
emma.mariaComment #51
emma.mariaComment #52
emma.mariaOK 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 :)
Comment #53
rachel_norfolkwill sort...
Comment #54
rachel_norfolkComment #55
rachel_norfolkComment #58
emma.mariaRerolled plus removed the extra declaration of
buttons.css
that snuck into the libraries file which is unrelated to this issue.Comment #59
emma.maria#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
Comment #60
star-szrPHP/Twig review :)
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'])) {
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';
This needs to end in a colon per https://www.drupal.org/node/1354#lists.
Remove @ingroup themeable, that should only be on core templates.
Minor: Should be a space after the comma.
Comment #61
hussainwebFixed for comments in #60. I did a different kind of implementation for point 2. Let me know what you think.
Comment #62
emma.mariaComment #63
rachel_norfolkThey're definitely all the same height - yay!!!
Checked Firefox, Chrome and Safari.
https://www.drupal.org/files/issues/all-same-size.gif
Comment #64
emma.mariaThanks @rachel_norfolk :)
Can anyone do a follow up PHP/Twig review of the improvements in #61?
Comment #65
lauriiiTwig/PHP looks good.
Comment #66
alexpottNeeds a reroll.
Comment #67
rachel_norfolkI'm on it...
Comment #68
rachel_norfolkAh 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.
Comment #69
lauriiiStill looks good! Tested manually on Bartik home page and no visual changes! :)
Comment #70
alexpottCommitted 997da33 and pushed to 8.0.x. Thanks!
Fixed incorrect file modes on commit.
Comment #73
emma.mariaComment #75
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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:
That
isset()
check looks redundant to me, unless I am missing something?http://drupal.stackexchange.com/questions/206846/twig-template-for-searc...
Comment #76
Jeff Burnz CreditAttribution: Jeff Burnz commentedNo idea why that changed...