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

sun - June 3, 2008 - 15:21

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

Gábor Hojtsy - June 3, 2008 - 15:32

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

sun - June 3, 2008 - 16:15

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

sun - June 8, 2008 - 14:12

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.

AttachmentSize
admin_menu.margin-top.patch 1.06 KB

#5

pwolanin - July 20, 2008 - 19:47
Status:active» needs review

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).

AttachmentSize
margin-top-per-theme-266099-6x-5.patch 3.76 KB

#6

sun - August 11, 2008 - 00:12
Assigned to:Gábor Hojtsy» Anonymous

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:

Apply margin-top to page body
By default, the output of this site will be shifted for approx. 20 pixels from the top of the viewport to make room for the Administration Menu. This, however, might break the layout in certain themes that use absolute or fixed positioned page elements. If disabled for a given theme, page elements at the top of the viewport may be covered by Administration Menu.

Also, the caption is arguable.

AttachmentSize
admin_menu-DRUPAL-6--1.margin.patch 3.66 KB

#7

sun - September 18, 2008 - 22:42
Title:Why do we have that "Apply margin-top to page body" setting at all?» Allow to configure margin-top setting per theme
Category:task» feature request

#8

alienbrain - April 8, 2009 - 00:32

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 :absolute which returns absolute-positioned elements. Then we modify them to add +admin_menu's height to their top attribute.

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

alienbrain - April 9, 2009 - 12:12

I think it wasn't the right place, I've started a new issue: http://drupal.org/node/428844

#10

mcrittenden - June 17, 2009 - 20:24

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?

 
 

Drupal is a registered trademark of Dries Buytaert.