Allow to configure margin-top setting per theme
Gábor Hojtsy - June 3, 2008 - 13:01
| Project: | Administration menu |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Description
I am wondering why is there this lone setting to "Apply margin-top to page body". The help page and the explanation for the setting explains that this setting is to be used to fix certain theme issues. But it does not mention that you would have any issues if this setting is enabled (and it is enabled by default)?
Why do we need to have this setting at all? I think it is unnecessary complexity.

#1
Is this a usability issue?
See #214740: Body top margin breaks some layouts for further information about this setting. Actually, it's very useful and totally depends on the theme one is using.
#2
I think this is a usability issue indeed. It is not explained why it would be a problem to turn that setting on (and it is turned on by default). Why it would be an issue to turn it off is explained.
After reading the explanations, I fully agree that this should be a per theme settings (as also discussed in http://drupal.org/node/214740). This could be needed differently based on the theme being used. That would hide this setting a bit, but I doubt the issue is widespread, since most themers use wrapper divs inside the body to form the page content (even though that adds a bit of extra markup), and that makes admin_menu work without a glitch for most themes.
#3
To be more specific on the original issue: The originally hard-coded margin-top breaks a theme if there's at least one element positioned absolute (and it's not contained in another element that is positioned relative). So this issue actually happens a lot of times.
However, wasn't there a new theme settings API in D6 (I'm currently unable to find in the API docs) that would simplify moving this setting to the theme configuration page?
We would keep the settings though, as I'm already working on several other optional enhancements, that need to be configurable on the settings page.
#4
As far as 5.x is concerned, I've committed the change in attached patch to explain this setting better.
For 6.x, we should really turn this into a theme setting.
#5
To make the checkboxes code simple and keep the current behavior of adding the margin-top to any theme, this patch uses some inverse logic (omit rather than apply).
#6
From a usability perspective, negated configuration options should be avoided at all costs. So I've reverted that change in the patch, and fixed some E^ALL issues in the update function.
However, I think that Gábor's original issue is still not properly fixed. I can see the chance of a big question mark in a user's mind when looking at this configuration option.
Given the fact that I needed to disable this setting in my last 2 or 3 projects, I'd simply vote to disable the setting by default IF it would not "break" 95% of all other themes. ;)
Anyway, we should be able to fix this by optimizing the label and description. A try:
Also, the caption is arguable.
#7
#8
I just had this crazy idea, the goal is to adhere to what I consider admin_menu's unwritten rule since I installed it few weeks ago: Just Works (TM).
*me hopes this is the right place for this suggestion*
Since absolute positioned elements are always relative to the viewport, we can be *sure* that if we want to add admin_menu at the top it means like every other absolute position element to be shifted by admin_menu's height. Anybody up for some jQuery-fu?
We can extend jQuerys selector to add something like
:absolutewhich returns absolute-positioned elements. Then we modify them to add +admin_menu's height to theirtopattribute.Just an idea proof, try this in your firebug:
jQuery.extend(jQuery.expr[':'], {absolute: "jQuery(a).css('position')=='absolute'"}); $(":absolute").each(function () { var top = $(this).css("top"); if (top != "auto" && top != "inherit") $(this).css("top", parseInt(top.replace("px", "")) + 20 + "px"); });#9
I think it wasn't the right place, I've started a new issue: http://drupal.org/node/428844
#10
I know I'll probably just get overruled here, but I think we're better off just avoiding this altogether, and adding a tip like "if you need to adjust the margins when admin menu is active, do so in the CSS using a rule such as '.admin_menu #page { margin-top: 30px; }'" I don't think it's the job of this module to cater to every weird thing that could be going on in a theme. I think themers expect to have to make a couple adjustments when adding stuff like this to the DOM.
Thoughts?