Screenshot illustrates this issue. Green is the container; red is the menu block. The padding above the menu block shouldn't be there.

@sheena_d was able to fix the issue with two changes:

  1. Add a clearfix to the menu's <ul> tag.
  2. Remove margin-bottom from menu's <li> tags.

Removing the margin-bottom will probably cause a side effect of not dropping down elegantly . This issue may need to be paired with some menu targeting in responsive CSS.

CommentFileSizeAuthor
#5 remove-excess-menu-space-5.patch812 bytesaquariumtap
#1 remove-excess-menu-space.patch859 bytesaquariumtap
Screen Shot 2011-11-13 at 3.32.14 PM.png12.21 KBaquariumtap

Comments

aquariumtap’s picture

aquariumtap commented
Assigned: Unassigned » sheena_d
Status: Active » Needs review
StatusFileSize
new remove-excess-menu-space.patch859 bytes

Ahh, I found a fix that doesn't require removing the margin-bottom from <li> tags. If the menu block is no longer floated left, the issue goes away. There shouldn't be any undesirable side effects.

I've added this to fusion-style.css:

#main-menu .block {
  float: none;
}

See attached patch. Marking as "needs review" since a change to fusion-style.css will affect all subthemes. We're still in alpha for 2.x, so I think this is acceptable change and relatively low impact. Alternatively it can be added to the starter kits, but it feels core-ish to me.

sheena_d’s picture

sheena_d commented

Doesn't removing the float on #main-menu .block assume that no one will even want to float two blocks beside each other in the main-menu region?

aquariumtap’s picture

aquariumtap commented
Status: Needs review » Needs work

For the most part, yes that's my assumption, although a "region" seems to lose its purpose if the theme only anticipates a single block. And I don't like the idea of blocks behaving differently by region.

This would be easier to override, and has the same effect:

.region-main-menu .block {
  float: none;
}

Alternatively, the main menu could be treated like it is in Bartik. 

    <?php if ($main_menu): ?>
      <div id="main-menu" class="navigation">
        <?php print theme('links__system_main_menu', array(
          'links' => $main_menu,
          'attributes' => array(
            'id' => 'main-menu-links',
            'class' => array('links', 'clearfix'),
          ),
          'heading' => array(
            'text' => t('Main menu'),
            'level' => 'h2',
            'class' => array('element-invisible'),
          ),
        )); ?>
      </div> <!-- /#main-menu -->
    <?php endif; ?>
stephthegeek’s picture

stephthegeek commented

If we remove the float in this region, aren't we then ending up with a region where blocks behave differently?

A big reason for going to blocks in D7 was to allow multiple blocks in the main menu region -- commonly something like floating a search box, user account links, language switcher, etc. to the right.

I believe when I've run into this in the past, I've targeted the ul:

.region-main-menu ul {
  margin-bottom: 0;
}
aquariumtap’s picture

aquariumtap commented
Status: Needs work » Needs review
StatusFileSize
new remove-excess-menu-space-5.patch812 bytes

Yup, that was my point. The solution I posted needed more thought.

@stephthegeek, your fix worked for me. Yay! Strange that margin-bottom: 0 on the <ul> shifts the block up, rather than removes what's below. I guess that's an effect of the float within a float?

New patch attached. So, where should this live? I have it in fusion-styles.css. It's easy to override, so shall it stay there? Or do we play it conservatively and put it into starter theme CSS?

stephthegeek’s picture

stephthegeek commented

Yeah, basically with the floats, the <ul> doesn't think it has anything in it, so even its bottom margin hangs out at the top.

I agree this should go in core, not the starter theme... I've had to override it several times (and it's confusing for people to figure out) so it seems a sane default, plus as you said, it's easy to override.

aquariumtap’s picture

aquariumtap commented
Status: Needs review » Closed (fixed)

Ahh, thanks for the explanation. Light bulb above head. Committed to dev branch, see 27cf43c.

  • Commit 27cf43c on master, 7.x-2.x by aquariumtap: 
    Issue #1340348: corrects excess spacing above main menu.