Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The default search block could use some help. There is a lot of unused space on the right side of the block. I would recommend either making the textfield fill the width of the block, or make the search button and the text field inline with each other.
Attached screenshot shows that it currently looks like.
Comment | File | Size | Author |
---|---|---|---|
#62 | bartik-search-form-issues.png | 23.87 KB | Jeff Burnz |
#62 | bartik-search-buttons.png | 6.84 KB | Jeff Burnz |
#62 | improve-search-forms-777728.patch | 3.24 KB | Jeff Burnz |
#54 | search-block.patch | 2.11 KB | bleen |
#53 | search_box_IE6_IE7.png | 1.72 KB | aspilicious |
Comments
Comment #1
watsonerror CreditAttribution: watsonerror commentedmaking form buttons in the sidebars a little smaller
Comment #2
watsonerror CreditAttribution: watsonerror commentedsmaller buttons and text fields in sidebars
Comment #3
watsonerror CreditAttribution: watsonerror commentedComment #4
Jeff Burnz CreditAttribution: Jeff Burnz commentedThe changes are good, can you please make it work for the footer columns also. The search button looks a little scrunched but imo its better than before with it wrapping under.
FYI your patch index is wrong, it needs to be
Index: css/style.css
, I changed it to get the patch to apply.Comment #5
watsonerror CreditAttribution: watsonerror commentedthanks,
Jenn suggested using a magnifying glass icon like the one in the Firefox search bar.
I'm going to try that and post a screen-shot. That way the other buttons in the sidebar aren't affect by this change.
Comment #6
watsonerror CreditAttribution: watsonerror commented@Jeff Burnz
Button change applied to footer too. I also set a min width of the button for shorter words so the button doesn't begin to appear round.
Comment #7
Jeff Burnz CreditAttribution: Jeff Burnz commentedAre you gonna go for the magnifying glass icon idea - that sounds like it would work out really nice with this theme.
Comment #8
eojthebraveWe'll either need to find a GPL icon, or someone will have to make one. Which, I suppose wouldn't be to hard. Just a circle with line coming out of it.
Comment #9
watsonerror CreditAttribution: watsonerror commentedyes I'll make the icon myself.
Comment #10
Jacinechanging this to needs work, while waiting for the icon :)
Comment #11
watsonerror CreditAttribution: watsonerror commentedshould I add the button background to it? I would have to make it darker.
Comment #12
watsonerror CreditAttribution: watsonerror commentedI'm going to add the button background to it. It will look a lot better. attachment to come later tonight.
Comment #13
eojthebraveNice! I like the icon. Good work.
I think we should add some javascript that ads the word 'Search' to the field, then automatically clears it when the field gets focus. Like the search field here for example: http://digg.com/
Something like the following should do the trick (works in D6, haven't tested in D7).
Comment #14
bleen CreditAttribution: bleen commentedThis patch adds some javascript to implement @eojthebrave's suggestion ... its a bit different then the code in #13
Comment #15
bleen CreditAttribution: bleen commented@drhino ... is the patch in #6 supposed to be combined with the patch in #2, or is the patch in #6 supposed to be tested instead of #2?
In general if you submit a patch (like #2) and someone suggests you add to it, then next patch (like the one in #6) should include everything you did already plus the code you're adding.
Once I know what you are intending in #2 & #6 we can combine everything from all the patches.
Also, watch out for white space when you're writing code:
no whitespace at the end of lines ... and use two-spaces instead of tabs (http://drupal.org/coding-standards)
Powered by Dreditor.
Comment #16
eojthebrave@bleen18, awesome thanks!
Only comment on the javascript, we should probably use the javascript translation function.
Drupal.t()
for the word 'Search'. http://drupal.org/node/323109Otherwise looks good to me.
Comment #17
bleen CreditAttribution: bleen commentedgood point ... here we go
Comment #18
watsonerror CreditAttribution: watsonerror commentedcheck it out, this makes me happy.
Comment #19
watsonerror CreditAttribution: watsonerror commentedDoes anyone know what the blue area of the buttons sprite is for?
Comment #20
tlattimore CreditAttribution: tlattimore commentedLooking really good in the screenshots drhino. It's not quite looking the same on my end, I attached a screenshot of what it looks like for me. Could you roll a single patch against
HEAD
that includes all your changes to this. Also, could you attach a copy of the image you used for the magnifying glass?Comment #21
watsonerror CreditAttribution: watsonerror commentedI was trying to hack the IE bug fix in but I figured there's a higher probability to fix this if I send out the patch.
Comment #22
watsonerror CreditAttribution: watsonerror commentedoh I didn't clean up the css as requested earlier, sorry about that
Comment #23
watsonerror CreditAttribution: watsonerror commentedI fixed the IE bugs new patch to come soon
Comment #24
watsonerror CreditAttribution: watsonerror commentedIE bug fixed
Comment #25
tlattimore CreditAttribution: tlattimore commentedHey drhino, would really like to review the patch from #21, but I cant seem to get the patch to apply? I don't see any meta information pointing the bartik CVS repo? It looks like it is point to a git repository to check diff, odd.
I am still pretty new to creating and applying patches, am I doing something wrong?
Comment #26
bleen CreditAttribution: bleen commentedThe patch needs to be created from the root of the bartik folder ...
Also, once i manually edited the patch to try and review, it wouldn't patch properly... I think you need to update your version of Bartik from CVS Head and then reroll the patch.
Just looking at the code though .. this looks real close :)
Comment #27
watsonerror CreditAttribution: watsonerror commentedCool I'll get this out today, but I kept getting an error when checking out bartik from CVS.
Comment #28
bleen CreditAttribution: bleen commentedLet keep the issue title on task ... Ill try and find you in IRC (or you can always ask jen or jacine or johnalbin or lots of others about the error if Im not around
Comment #29
JohnAlbinHere's a re-rolled patch.
Comment #30
JohnAlbinHmm… this is very close to being perfect, IMO. But I think the text box needs to expand depending on the size of the region its in.
Here's a patch that accomplishes that.
Comment #31
JohnAlbinCommitted! http://drupal.org/cvs?commit=362792
Comment #32
watsonerror CreditAttribution: watsonerror commentedthanks JohnAlbin!
Comment #33
bleen CreditAttribution: bleen commentedThe search button is missing in iE6 and its half missing in IE6 RTL ... iE6 makes me sad
Comment #34
jensimmons CreditAttribution: jensimmons commentedThe search block looks awesome on some pages (like the home page), but on others there's too much space on the bottom.
And then, the search *page* is kind of a mess.
Comment #35
Jeff Burnz CreditAttribution: Jeff Burnz commentedIE7 also, somethings really wierd here, try editing a node and the button will show partly, but in other views it will not show, the block is changing height also (I did say weird...)
Comment #36
Jeff Burnz CreditAttribution: Jeff Burnz commentedHeres a different approach that uses position absolute instead of floating the div, which is the main problem here.
Needs RTL love...
Screenshot is IE7.
Comment #37
tlattimore CreditAttribution: tlattimore commentedLooks good in IE6-7. But as stated, RTL needs work. It's doing some really quirky things there.
Comment #38
Jeff Burnz CreditAttribution: Jeff Burnz commentedAt this eleventh hour I am not sure we need to nail all RTL issues, they are mostly minor, even 7 has major RTL issues as I recall. Its just a matter of a left/right here and there.
Comment #39
JohnAlbinOh, wow. I don't like position: absolute for this at all. Why do you think float doesn't work here?
Comment #40
Jeff Burnz CreditAttribution: Jeff Burnz commentedAbsolute is cool for positioning input buttons because you can lock it to the end of the text field, if the field gets wider it simply moves over (auto left/right values), but it will never wrap, which is bad and might break out/bleed over the edge of the block border etc, so yes, its bad-ass in that regard.
Frankly other positioning methods are more flexible and more well suited for this, I just lacked time to work it up properly and threw this out there for testing and feedback - just brain storming for the IE fix really.
Comment #41
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'm looking in IE7 and not seeing an search button on the search block.
Comment #42
Jeff Burnz CreditAttribution: Jeff Burnz commentedHeres a screenshot, this is critical.
Comment #43
jensimmons CreditAttribution: jensimmons commentedThis needs to get cleaned up, but it's not a critical bug. It's more of a must-get-to-this than a this-makes-things-explode.
Comment #44
jensimmons CreditAttribution: jensimmons commentedComment #45
jensimmons CreditAttribution: jensimmons commentedHere's a weird problem, the search block has more padding on the bottom on some pages on the site than on others.
Home page, page page:
Article node page, Contact Form page:
What's up with that? Let's fix it.
Comment #46
bleen CreditAttribution: bleen commented...and if you look really close at the screenshots in #45, there is more space between the input and the button in the first screenshot
Comment #47
jensimmons CreditAttribution: jensimmons commentedOh, and we must fix the bug Jeff reported in #41-42. We need a search button in IE7!
(I just meant that it wasn't 'critical' according to the definition we were using on the Bartik queue, where critical meant it had to be fixed *before* Bartik to go into core. It's definitely a big deal and must be fixed.)
Comment #48
bleen CreditAttribution: bleen commentedThe problem is that the search form (correctly) has a different #ID in different contexts. This is correct because it can appear more than once on a page. There were a couple of styles that were aimed at the search block by #ID as it appears on the homepage and that caused the difference in styles.
Comment #49
bleen CreditAttribution: bleen commentedThis patch also fixes the search button in IE6 & 7
Its ugly css (for IE) but I couldn't make anything else work.
Comment #51
aspilicious CreditAttribution: aspilicious commentedis unrelated and already in ;)
Comment #52
bleen CreditAttribution: bleen commentedlast patch had some cruft
Comment #53
aspilicious CreditAttribution: aspilicious commentedYour patch has a
So I rolled a new one, I just rerolled with a extra newline.
This is rtbc, I got a screenshot to confirm ;)
Comment #54
bleen CreditAttribution: bleen commentedThis is a slightly better patch because the search bar and button are more nicely centered in the block in IE7 with this patch
Comment #55
aspilicious CreditAttribution: aspilicious commentedConfirmed
Comment #56
ff1 CreditAttribution: ff1 commentedConfirmed. IE7 looks much better with the latest patch. Also tested in firefox.
Comment #57
bleen CreditAttribution: bleen commentedbump
Comment #58
Jeff Burnz CreditAttribution: Jeff Burnz commentedIn its current state the magnifying glass icon fails WCAG2 contrast requirements - afaikt its #737373 on a gradient background (top around #f2f2f2, bottom around #ececec - just the bit of the gradient that the icon sits over).
If the little magnifying glass is darkened to #666 or darker it passes.
Maybe this is a different issue, sorry if this is creep, maybe we can just nip this one here?
Comment #59
aspilicious CreditAttribution: aspilicious commentedGoing to put it back to rtbc.
#58 is another issue. Made a new one ==> #889660: Magnifying glass need to be darkened
Tried to edit the picture myself but it didn't work out as it hasn't have one solid color.
This patch is a bartik major issue, so I want to get it in as soon as possible.
Srry Jeff!
Comment #60
Jeff Burnz CreditAttribution: Jeff Burnz commentedRight on, lets get this in a move on, +1.
Comment #61
Jeff Burnz CreditAttribution: Jeff Burnz commented#54: search-block.patch queued for re-testing.
Comment #62
Jeff Burnz CreditAttribution: Jeff Burnz commentedSetting back to needs work - there are a number of issues with search forms even with this patch. While the patch does solve the issue of the broken search submit button in IE7 after looking at the CSS more closely I can see most of the issues are actually caused by some of the original CSS. There also appears to be a rather verbose work-around for text-indent not working correctly in IE.
I have attached a screen-shot of the advanced search page - clearly we can see the search buttons are wrapping awkwardly and really the "Enter your keywords" label should probably be above the text input field.
The issue of the misplaced search button in the advanced search fieldset appears to be weight issue with the Languages criterion - they are priting below the search button so that is another issue.
The search buttons are also rather lost on the background color - they are not clearly defined. Why not give them a border? In IE without a border they are almost invisible - I can only see the magnifying glass in some color schemes - and since we've lost text resizing to make it bigger I think we need more impact here - a border will lift the visibility of the button.
I've taken some time to write up another approach to theming these search form inputs - it uses floats and negates much of the special casing required for IE and the various elements.
Note that this patch solves a couple of issues that the patch in #54 does not:
1) fixes the display of the search form on the search results page (places the label above the text input, which automagically aligns the button correctly also)
2) adds more definition to the button by using a border
3) correctly sets the height on buttons for IE so they are the same height as the text input, and sets a proper width on the search button - before it was relying on inherited padding which is fragile.
4) removes a fair amount of cruft from the CSS, especially the IE stuff
@bleen18 - have i missed anything here - there was a bit of "width auto" in there - although it was not clear to me what the search block is supposed to look like in the #block-system-main (e.g. in the highlighted region).
Comment #63
Jeff Burnz CreditAttribution: Jeff Burnz commentedbot...
Comment #64
bleen CreditAttribution: bleen commentedI've been playing with #62 for the last hour and I think it covers everything from #54 and a bit more .... looks good to me and I think its high time we fix this sucker... RTBC
Comment #65
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.