Posted by ao2 on January 31, 2008 at 4:30pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | Garland theme |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
When we set a void left panel, the bottom of the logo image overlaps with the top of the center DIV.
Steps to reproduce:
- Log in as Administrator
- Go to Administer -> Site building -> Blocks
- Move any items away from the Left sidebar region
- Save and logout
I would have expected no ovelaps. Have a look in the upper left part of the attached picture to see the current, wrong, behaviour.
Please, even if this is a really trivial bug, and people are going to replace the image logo, it would be nice to have that fixed before 6.0
Thanks,
Antonio
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal-garland-logo-overlap-when-no-leftpanel.png | 94.17 KB | Ignored | None | None |
Comments
#1
This will pretty much fix it but there is a very, very subtle color mismatch due to the gradient. If we wanted to get rid of that too, the base.png and logo.png graphics would have to be tweaked.
This is also present in Minnelli.
#2
http://drupal.org/node/216848 was duplicate.
#3
Yes, it looks good to me, don’t notice the mismatch. But with a left sidebar I think the logo is noticeably too high.
This makes the same change but conditional on left sidebar presence. I’ve put a class on #header, in case anything else wants to depend on it.
#4
1.)
<body>already puts out the layout classes.2.) Is it the height really that noticeable?
IMO, It's not needed.
#5
This is not such a big change. Tested both themes, with and without right and left sidebars.
#6
this bug is still here in 6.11.
If it is not going to be fixed, can you devs at least mark it "won't fix"?
Thanks,
Antonio
#7
Even in drupal7
#8
does the CSS really need to be this specific?
+body.sidebar-left #wrapper #container #header h1 img, body.sidebars #wrapper #container #header h1 img {+ padding-top: 16px;
+}
#9
Bumping this, cause it is still an issue.
If I put this in markup I'm sure at least one person will read this.
#10
This doesn't belong in markup :P
#11
subscribe
#12
sub
#13
No, it doesn’t really need to be so specific – but there is a history;–
body.sidebars #wrapper #container #header h1 img {
and a future;–
body.sidebars #branding img {
Also code has moved, so will attach a new patch…
#14
The patch in #5 would now equate to...
#branding img {padding-top: 10px;
padding-right: 20px; /* LTR */
float: left; /* LTR */
}
While this "fixes" the issue it also creates a new one - the logo in Garland is colourized (the gradient is part of the logo) - I would suggest switching to a logo with a transparent background so we can use the above fix without gradient matching issues. While this wont be so nice in IE6 I do not think this is much of a big deal since support for IE6 only need to be to the extent that the theme is not "broken", this is merely a cosmetic difference IMO (Bartik uses a transparent logo).
#15
OK, here’s the patch, and a logo with transparency.
I believe there is a fix for png’s in IE6.
This patch restores the original position of the logo and moves it up by 6px if there is no sidebar below it.
Can someone test RTL on their setup? Also I’m not a graphics expert, so feel free to redo the logo, I won’t be offended :-)
#16
If someone could size up an icon-logo from scratch that would be good (see http://drupal.org/node/9068) - the one in the patch looks pretty rough :)
#17
I’ve resized the druplicon (cubic interpolation) from http://drupal.org/node/9068 (not from scratch) and carefully avoided placing it in a patch.
And apart from being 1K smaller, I can’t see any great difference – which confirms my suspicions that I’m not a graphics expert,
#18
OK, lets go for it, RTBC for the patch in #15 and the new logo in #17.
#19
+++ themes/garland/style.css 2008-02-02 00:54:15.000000000 +0000@@ -458,11 +458,16 @@
+body.sidebar-first #branding img,
+body.two-sidebars #branding img {
1) The additional body selector should be removed.
2) Stray duplicate space in the second selector.
Powered by Dreditor.
#20
+body.sidebar-first #branding img,+body.two-sidebars #branding img {
+ padding-top: 16px;
+}
+
/* With 3 columns, require a minimum width of 1000px to ensure there is enough horizontal space. */
body.two-sidebars {
Works as is. Note 3rd body at end of patch.
#21
Are you thinking of an id such as
h1#brandingwhich doesn’t need the tag name as there is only ever one tag on a page with a specified id?This doesn’t apply to classes.
#22
There only every should be one instance of an id on the page, but that's not the only reason to not use a tag#id or tag.class selector.
The style that those declarations override is #branding img. You only need to add the classes. Adding the tag.class is overly specific, and therefore unnecessary.
#23
Thanks James for your explanation, but I think I didn’t add sufficient emphasis to #21: …there is only ever one tag on a page with a specified id? therefore the tag is uniquely identified (pun intended) and further specification is redundant.
It is misleading to say it is not the only reason to not use a tag.class selector, because in this case, it isn’t a reason at all, is not true and it doesn’t apply.
You say tag.class is overly specific, but this contrasts with ‘further specification is redundant’ as opinion of relative degree with absolute fact:
If a browser finds the tag specified by
#branding img(a simple task) it then has to check each surrounding tag in turn to see if it has the specified class, until the body tag is reached. There may be more efficient algorithms than simplistic iteration, but surely informing the browser that it can go straight to the body tag (whether it uses this information or not) is the best – and avoids the interference of intervening tags with these classes, however remote the possibility of their existence.Minimising specification is not the only reason to not use a tag.class selector – it works with and without – with is better: both for execution in the present, and code maintenance in the future.
Setting back to rtbc as posted by Jeff Burnz in #18. If you disagree please post a patch and set to cnr.
@sun Did you mean to advocate the removal of both body selectors?
#24
#15: no_left_sidebar_III.patch queued for re-testing.
#25
The image in #17 is totally wrong; it should be using the Druplicon from Garland's base.png, which is already the correct size and has a subtle (but important) dark stroke.
Also, since Garland's logo area is broken for custom logos, I'm not sure that fiddling with the placement of the logo to make the default logo look right is the correct approach.
#26
Logo from base.png.
padding-top was originally 16px, and is now 14px. Due to the alpha, this logo doesn’t need to go as high as 10px as the previous patch specifies – 14px looks OK, but I think 12px looks better. So we could ‘totally’ forget the patch – always 14px – or use no_left_sidebar_IV.patch to restore original 16px and lift to 12px when the left sidebar disappears.
#27
Garland's logo area works at least for custom logos that are a similar size to the default. This issue fixes one wrinkle to improve a core theme. I’m not trying to be Gordon Ramsey’s plastic surgeon.