With the advent of the new Stark theme, we can now easily see the output of Drupal 7's default html. The page.tpl.php is in need of refactoring, and some css associated with it may also need altering/updating to match any changes made to the page template.

We need to stay as focused on the page as possible. New issues will be filed for each template as we finish the previous one. We'll probably go in this order:

page
node
block
comment
box

So let's focus on just the individual tpl file we're working on. This is meant to be a big bikeshed over page.tpl.php so that all interested parties can get their say so on the default output. If you'd like to follow along, cvs out the newest version of Drupal 7 and load up the stark theme or on D6 you should get roughly the same thing if you'd like to use this: http://drupal.org/project/stark

Let the bikeshed begin.

Eclipse

Files: 
CommentFileSizeAuthor
#96 page_patch_367299_2.patch11.16 KBEclipseGc
Unable to apply patch page_patch_367299_2_1.patch
[ View ]
#95 page_patch_367299.patch11.04 KBEclipseGc
Passed: 10268 passes, 0 fails, 0 exceptions
[ View ]
#91 drupal-page-tpl-367299-91.patch11.02 KBJohnAlbin
Unable to apply patch drupal-page-tpl-367299-91.patch
[ View ]
#89 page.tpl_.php.txt7.58 KBgeerlingguy
#86 drupal-page-tpl-367299-86.patch11.21 KBJohnAlbin
Unable to apply patch drupal-page-tpl-367299-86_0.patch
[ View ]
#81 drupal-page-tpl-367299-81.patch14.89 KBJohnAlbin
Failed: 9559 passes, 189 fails, 8319 exceptions
[ View ]
#79 drupal-page-tpl-367299-79.patch15.93 KBJohnAlbin
Unable to apply patch drupal-page-tpl-367299-79.patch
[ View ]
#70 drupal-page-tpl-367299-70.patch13.88 KBJohnAlbin
Failed: 9758 passes, 10 fails, 0 exceptions
[ View ]
#67 drupal-page-tpl-367299-67.patch12.91 KBJohnAlbin
Failed: 9727 passes, 8 fails, 0 exceptions
[ View ]
#64 drupal-page-tpl-367299-64.patch12.03 KBJohnAlbin
Failed: Failed to run tests.
[ View ]
#43 drupal-page-tpl-367299-43.patch10.98 KBJohnAlbin
Failed: Failed to apply patch.
[ View ]
#41 drupal-page-tpl-367299-41.patch7.72 KBJohnAlbin
Failed: Failed to apply patch.
[ View ]
#26 page.patch3.87 KBEclipseGc
Failed: Failed to apply patch.
[ View ]
#25 page.patch3.88 KBEclipseGc
Failed: Failed to apply patch.
[ View ]
#7 page.tpl_.php_.patch7.49 KBgeerlingguy
Failed: Failed to apply patch.
[ View ]
#6 page.tpl_.php_.patch7.54 KBgeerlingguy
Failed: Failed to apply patch.
[ View ]
#5 page.tpl_.php_.patch7.58 KBgeerlingguy
Failed: Failed to apply patch.
[ View ]

Comments

Todd Nienkerk’s picture

Summary of chat with EclipseGc and JohnAlbin in #drupal. We want to:

  1. switch class name .clear-block to .clearfix for semantic purposes (in Drupal "block" connotes many things!)
  2. remove #main-squeeze from page.tpl.php and use a standard .inner class that we can place in #page, #main, .node (maybe), etc.;
  3. standardize the use and naming of three classes that any theme will need: .column, .inner, .content (.content in particular is useful for nodes and blocks)
  4. make sure a .inner inside another .inner doesn't inherit too much garbage (like #page > .inner > ... > #main > .inner)

To further clarify point #4, here's an example where nested .inner divs could cause a problem:

<div id="page"><div class="inner">
  I am the page! My inner div provides some padding.

  <div id="main"><div class="inner">
    The main body of the page goes here (nodes, tabs, etc.).

    This area can be a problem because it's inheriting properties from both #page .inner AND #main .inner. Why? Because the "#page .inner" selector means "any .inner class inside #page," which applies to #main .inner as well because #main .inner is inside #page.

  </div></div><!-- /.inner /#main -->
</div></div><!-- /.inner /#page -->

JohnAlbin’s picture

yes, the nested .inner classes could be a real headache.

Its too bad we can't use the first-child pseudo-class. :-( Stupid IE. Wait, a sec… It's only IE6 (and below) that doesn't support :first-child. And, we may be dropping IE6 support in Drupal 7…

What do you think? or are #page>.inner:first-child and #main>.inner:first-child too hard to grok?

EclipseGc’s picture

I would love to switch to the first-child psuedo class, does 7 support it? I know it doesn't support nth-child yet (otherwise we could dispense with even/odd classes). But if everyone besides IE6 understands first-child then I would at least contemplate this (though I think we'll probably choose a different method as IE6 is probably going to be around another 2-3 years...)

I'm actually in favor of not having an inside the #page div. I don't see a real reason for it, we'd seldom (if ever) use the #page div in any way that would require additional margin/padding, and the main reason for having inner divs is for %-based layouts that need margins between their regions. (as you can't do a 20% width and 10px margins since they're additive) I don't see any reason the page is going to need this really. And if you do need it, that's why our page templates can be over-ridden.

Thoughts?

JohnAlbin’s picture

Actually, I'm going to answer my own question as well. IE7 does support :first-child, but #page>.inner:first-child is too hard to grok. Its elegant, but non-intuitive.

Also, what are the chances you'll want to put the exact same padding/styles on the #page's inner as on the #main's inner. It seems more likely you would want the same styles on each of the columns. So, can we do #page, #page-inner and then have #left-sidebar.column, #left-sidebar.column-inner?

Crap, plus we forgot about the content-first bikeshed. ok. let me start that one rolling: Content-first is better semantically and better for the SEO-inclined. etc. etc. sigh. Ah, man, I think I need some tea before defending that position. :-p

geerlingguy’s picture

Status:Active» Needs work
StatusFileSize
new7.58 KB
Failed: Failed to apply patch.
[ View ]

This patch adds a wrapper div around the page div, which is common practice for *many* websites that want to simply center their content in a way compatible with IE, FF, Safari, etc.; it also allows for things like adding a shadow effect to the sides of the page area...

As a CSS-only kind of guy, this would be quite helpful for making an amazing CSS-only theme.

I'm assigning "patch (code needs work)" because there will be a few other changes before page.tpl.php is put to rest, I am sure.

geerlingguy’s picture

StatusFileSize
new7.54 KB
Failed: Failed to apply patch.
[ View ]

Fixed the spacing issue on the patch (silly me... first time ever patching something!):

geerlingguy’s picture

StatusFileSize
new7.49 KB
Failed: Failed to apply patch.
[ View ]

I made a few more changes in this patch to page.tpl.php, incorporating some of the suggestions above, which I'll outline below:

1. Still have the page-wrapper div in from earlier patch.

2. Put content div before navigation, search, etc., and after title (SEO lovers, rejoice!). IMO, it's best practice to have content > nav/sidebars.

3. I took out the main-squeeze div. Not only have I never heard of this 'squeeze' thing (I always wondered why it was called main-squeeze anyways), but I agree with Todd's reasoning above. Plus, for the people that use inner divs (I think they're fewer than those who don't), they can make their own inner div naming system ;-)

4. I changed the .clear-block's to .clearfix (3 occurrences), to follow common CSS standards. I think the clearfix is probably the simplest way to clear divs for now, because of that evil browser known as IE.

dmitrig01’s picture

If we aren't dropping IE 6 support, that means we can't do the .inner class, because the child selector is not existant.

eaton’s picture

The details of good classnames are a little over my head, but would something like #left-inner be preferable to #left > .inner? Feel free to say no. Just considering...

EclipseGc’s picture

@dmitri,

I think we've chosen to use a method that won't require any first-child psuedo-classes, so no worries there. However we can still do easy stuff like #left .inner as an override.

@eaton,

the problem with doing a #left-inner id is that you now explicitly have to state it if you want any styling on it. What I'm proposing we do is thus:

  <div id="left">
    <div class="inner">
      <?php print $left; ?>
    </div>
  </div>
  <div id="main">
    <div class="inner">
      <?php print $content; ?>
    </div>
  </div>
  <div id="left">
    <div class="right">
      <?php print $right; ?>
    </div>
  </div>

And this will buy use greater flexibility because we can set .inner once, and then over-ride as needed. so in some cases this my suffice:

  .inner {
    margin:0 5px;
  }

Which would give us a 5px space on either side of the left and right regions with a 10px gutter between them and main. But in other situations, if you needed to lay it out differently you could:

  #left .inner {
    margin:0 10px 0 0;
  }

  #right .inner {
    margin:0 0 0 10px;
  }

Which would give us 10 px gutters still, with no margins between the right/left and whatever div contains them. In this way we still maintain the flexibility of what would be #left-inner but we also gather the flexibility to simply set .inner on a "global" scale. This means less css code unless we need to be more specific, in which case, we've provided a very basic, easy to use method of handling that issue.

Does my logic make sense here? Anyone have issues with it?

Eclipse

catch’s picture

My only concern with that is that while it works for left/center/right, it won't work for block and node templates down the line - because then we're back to nested .inner. So my vote would be for page-inner, left-inner, right-inner, block-inner even if that means slightly more CSS up front - much better than people having to do.

#sidebar_right .block .inner {
  margin: 0;
}

to remove inherited stuff later on.

EclipseGc’s picture

Catch,

There are a number of options here, not the least of which is simply not putting inner classes on individual blocks or nodes, however even if we choose to put inner classes on blocks and nodes we could solve that with a single statement

.inner .inner {
  margin:0;
}

Now, I'm NOT in favor of this, as I think that we'd better served by .block-inner and node-inner classes for blocks and nodes. We could go with region-inner as opposed to JUST inner. This still allows us the classes as I'd like to see them, the other option is that it's possible (likely even) that we'll end up with region classes on all regions anyway, in which case we could do .region .inner... in any case, I think there are a LOT of potential solutions here. We've just been saying ".inner" up until now.

I actually like the idea of .region-inner, .block-inner, .node-inner best thus far. Does anyone have issues with this class naming structure? or anything I said here.

Eclipse

webchick’s picture

.region-inner, .block-inner, and .node-inner sound great to me. What about pages that are not node pages, such as user profiles, administrative pages, etc? Just #content .region-inner ?

catch’s picture

I also like .region-inner .block-inner and .node-inner. Good plan.

EclipseGc’s picture

@webchick,

I would say "YES". :-) couple reasons there.

  1. The page tpl can't be responsible for how $content handles it's output
  2. I think we'll find that, specifically node-inner and block-inner are convenience classes, and will be used less often than region-inner. Region-inner will be taken advantage of in ANY liquid layout designs as it's the easiest way to get margins on variable sized content regions.

With that in mind, I would maintain that this is a good plan until proven otherwise. :-)

If no one has issues with this, then I'll format the page, alter stark's layout.css and make the other changes that have been mentioned and see if we can get a patch here shortly.

Eclipse

EclipseGc’s picture

An additional topic I'd like to discuss...

Typical themes seem to place the breadcrumb, and the messages within the #content region. I really hate this approach, and would like to move BOTH of these items outside the #content div and into the main #page div. Does anyone have an issue with this?

Eclipse

geerlingguy’s picture

@ EclipseGc - I really wish the breadcrumb function was just a block, that you could stick anywhere you darn well please. Half the sites I make don't use breadcrumbs, and the other half I have to move it to another region anyways... But your way would probably work fine.

EclipseGc’s picture

I don't really disagree with that, but it's outside the scope of this issue. So, still calling for anyone who has issues with moving breadcrumbs and messages. :-)

Todd Nienkerk’s picture

@ EclipseGc: I agree with moving breadcrumb and messages outside of #content -- maybe mission, too?

@ All: I love the .divtype-inner approach. To further bikeshed -- :) -- how do we feel about the .content class? Will nesting work here (.block .content, .node .content), or do we want some .divtype-content action?

add1sun’s picture

I'd agree with moving breadcrumbs, et al. out of the content div and I think the *-inner class approach is really sane. I'll have to ponder the *-content class a bit. I'm mostly replying right now to subscribe without being completely annoying. :-p

EclipseGc’s picture

Well, my thought on content is this:

either a.) we have no use for it (block-inner and node-inner basically perform the same task) OR,
b.) we can ditch block/node-inner in favor of .content classes there OR,
c.) we can have all those classes together, in which case I would propose:

current code:

<div id="main" class="column"><div id="main-squeeze">
        <div id="breadcrumb"><div class="breadcrumb"><a href="/drupal7testing/">Home</a></div></div>       
        <div id="content">

          <h1 class="title" id="page-title">Test page</h1>          <div class="tabs"><ul class="tabs primary">
<li class="active" ><a href="/drupal7testing/node/1" class="active">View</a></li>
<li ><a href="/drupal7testing/node/1/edit">Edit</a></li>
</ul>
</div>                              <div id="content-content" class="clear-block">
            <div id="node-1" class="node clear-block">

  <div class="meta">

 
    </div>

  <div class="content">
    <p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin sit amet arcu. Aenean cursus, orci nec sagittis semper, pede purus varius turpis, nec iaculis nunc nisl quis quam. Aliquam eget nisl vel pede molestie fringilla. Cras molestie. Vestibulum vitae ipsum eget velit suscipit tempus. Nam id sapien. Nulla et nulla. Phasellus at est at lacus posuere interdum. Donec velit sem, viverra sed, interdum a, vulputate nec, dui. Donec lectus mi, egestas eget, rutrum euismod, facilisis quis, neque. Fusce sodales. Aenean aliquam, dolor id sodales mattis, tellus libero hendrerit ipsum, a egestas quam dolor sit amet velit. Maecenas facilisis scelerisque urna. Sed eu odio. Donec at eros. Nulla quis ligula. Sed purus.</p>
<p>Curabitur sit amet dolor id velit gravida dictum. Proin vel sem. Integer mauris. Curabitur fermentum quam eu mauris. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Morbi ante. Aenean ornare magna a nisi. Proin vitae pede. Pellentesque porttitor, lectus id ornare sagittis, lorem sem condimentum justo, non semper enim magna non nunc. Curabitur lacus risus, pretium quis, fringilla ac, vestibulum et, mi. In congue pulvinar diam. Maecenas condimentum nulla vel felis. Morbi enim dolor, dictum non, suscipit vitae, rhoncus nec, nisi. Suspendisse ipsum sapien, feugiat ut, porttitor consequat, convallis eu, orci. Praesent mauris. Ut quam felis, semper eget, convallis vitae, fringilla vel, ipsum. Integer lectus lacus, ultrices id, aliquet facilisis, commodo at, orci. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos.</p>
  </div>

 
 
</div>          </div> <!-- /content-content -->

                  </div> <!-- /content -->

      </div></div> <!-- /main-squeeze /main -->

Proposition (cleaned it up for readability)

<div id="content" class="region">
  <div class="region-inner">
    <div id="node-1" class="node">
      <h1 class="title" id="page-title">Test page</h1>
      <div id="tabs-content"> <!-- I believe we only have one tabs div, so this should probably be an id, open to name suggestions -->
        <ul class="tabs primary">
          <li class="active" ><a href="/drupal7testing/node/1" class="active">View</a></li>
          <li ><a href="/drupal7testing/node/1/edit">Edit</a></li>
        </ul>
      </div> <!-- closing tabs-content -->
      <div class="node-content clearfix">
        <div class="meta"> <!-- would someone kindly enlighten me? -->
        </div>
        <p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin sit amet arcu. Aenean cursus, orci nec sagittis semper, pede purus varius turpis, nec iaculis nunc nisl quis quam. Aliquam eget nisl vel pede molestie fringilla. Cras molestie. Vestibulum vitae ipsum eget velit suscipit tempus. Nam id sapien. Nulla et nulla. Phasellus at est at lacus posuere interdum. Donec velit sem, viverra sed, interdum a, vulputate nec, dui. Donec lectus mi, egestas eget, rutrum euismod, facilisis quis, neque. Fusce sodales. Aenean aliquam, dolor id sodales mattis, tellus libero hendrerit ipsum, a egestas quam dolor sit amet velit. Maecenas facilisis scelerisque urna. Sed eu odio. Donec at eros. Nulla quis ligula. Sed purus.</p>

        <p>Curabitur sit amet dolor id velit gravida dictum. Proin vel sem. Integer mauris. Curabitur fermentum quam eu mauris. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Morbi ante. Aenean ornare magna a nisi. Proin vitae pede. Pellentesque porttitor, lectus id ornare sagittis, lorem sem condimentum justo, non semper enim magna non nunc. Curabitur lacus risus, pretium quis, fringilla ac, vestibulum et, mi. In congue pulvinar diam. Maecenas condimentum nulla vel felis. Morbi enim dolor, dictum non, suscipit vitae, rhoncus nec, nisi. Suspendisse ipsum sapien, feugiat ut, porttitor consequat, convallis eu, orci. Praesent mauris. Ut quam felis, semper eget, convallis vitae, fringilla vel, ipsum. Integer lectus lacus, ultrices id, aliquet facilisis, commodo at, orci. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos.</p>
      </div> <!-- close node-content -->
    </div> <!-- close node-1 -->
  </div> <!-- close region-inner -->
</div> <!-- close content -->

The naming conventions on some of this stuff is... confusing. I have a content id, a tabs-content id, a node-content class... frankly I'm not sure I like it. If we ultimately add the node-inner, then that would come immediately before the <h1> tag (for reference). I can see an argument for needing/wanting what I've currently labeled node-content. There are all sorts of good reasons that should be there, so after writing this, I think I too agree we need content classes, HOWEVER... I've gotten sidetracked off of page template. :-) So let's refocus on just that and we'll keep this stuff for the node/block arguments.

My main question after this is:

Should we change the id of the container for $content to "content" or leave it main? and go with main-inner? (which is WORLDS better than main-squeeze) but ultimately is providing the same thing.

Lemme know

Eclipse

geerlingguy’s picture

I'd go with main + main-inner. (Main squeeze has got to go!). I like main, instead of content... but with 'content', it would seem logical to include the breadcrumb and such other pieces of information besides nodes...

EclipseGc’s picture

What if we rename the regions to #left-region, #content-region, #right-region etc? We could potentially take off the region class I added because then it's obvious. (I and our breakout guy couldn't come up with a single time in the past where having a region class would be useful so... )

Enjoying the feedback.

Todd Nienkerk’s picture

@Eclipse: I like the -inner and -content combo you outline in #21. I think they serve very different purposes in terms of both styling/CSS (generating padding using -inner, etc.) and division of output (-content wraps the actual output of nodes, blocks, etc.).

I also like the idea of striking the .region class. I never understood the point, either. Who needs to style *regions* in a universal way?! :P I think .column and .sidebar suffice.

I further agree that "-region" should be added to #left, #right, and #content. After all, we have both #header and #header-region. #header-region looks like an oddball because it's the only "-region," but it's important to distinguish it from #header.

And while we're messing with "-region" divs, let's wrap $footer in #footer-region. It should be separated from $footer-message, which is often just a line of text with no wrapping markup. (At least the two are no longer concatenated! That probably annoyed me more than anything else in page.tpl.)

EclipseGc’s picture

StatusFileSize
new3.88 KB
Failed: Failed to apply patch.
[ View ]

So I've been working through these naming conventions a bit, here's what I've got at the moment. In addition to reworking some of the names, I've also reworked the html a bit, reducing where I deemed appropriate. Breadcrumb, and messages have been moved into the #page, I left mission statement alone as I need some more convincing yet.

I saved this as a .patch file, but this is just the page.tpl.php. Lots of css edits need to be worked in here, most of them within stark's layout.css and a little bit within default.css.

Please, fire away with comments.

EclipseGc’s picture

StatusFileSize
new3.87 KB
Failed: Failed to apply patch.
[ View ]

previous file broke primary links

geerlingguy’s picture

@ EclipseGc - could we possibly wrap #page in a #wrapper or .wrapper div?

Also, What are your thoughts on moving the content div above the left region, for SEO? It should probably stay below the messages and such, but I would love for content to be represented first... one of the primary reasons for separating form (CSS) from content is so that we can optimize the hell out of the content, then make it look pretty with CSS (by moving the columns where we need them).

Todd Nienkerk’s picture

I'd like to suggest some changes to #header-region and #footer-region.

First, the header. I think the entire header should be #header, and the wrapper around the region inside the header be called #header-region. (Also, I don't think the site name should be an h1 element for document hierarchy/SEO purposes. As I understand it, each page should only have one h1. The most appropriate h1 on a given page is #page-title.)

Current header area:

    <div id="header-region">
      <div class="region-inner">
      <?php if (!empty($logo)): ?>
        <a href="<?php print $front_page; ?>" title="<?php print t('Home'); ?>" rel="home" id="logo">
          <img src="<?php print $logo; ?>" alt="<?php print t('Home'); ?>" />
        </a>
      <?php endif; ?>
      <?php if (!empty($site_name)): ?>
        <h1 id="site-name">
          <a href="<?php print $front_page ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>
        </h1>
      <?php endif; ?>
      <?php if (!empty($site_slogan)): ?>
        <div id="site-slogan"><?php print $site_slogan; ?></div>
      <?php endif; ?>
      <?php if (!empty($search_box)): ?>
        <div id="search-box"><?php print $search_box; ?></div>
      <?php endif; ?>
      <?php print $header; ?>
      </div> <!-- /region-inner -->
    </div> <!-- /header-region -->

My suggested header area:

    <div id="header">
      <?php if (!empty($logo)): ?>
        <a href="<?php print $front_page; ?>" title="<?php print t('Home'); ?>" rel="home" id="logo">
          <img src="<?php print $logo; ?>" alt="<?php print t('Home'); ?>" />
        </a>
      <?php endif; ?>
      <?php if (!empty($site_name)): ?>
        <div id="site-name">
          <a href="<?php print $front_page ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>
        </div>
      <?php endif; ?>
      <?php if (!empty($site_slogan)): ?>
        <div id="site-slogan"><?php print $site_slogan; ?></div>
      <?php endif; ?>
      <?php if (!empty($search_box)): ?>
        <div id="search-box"><?php print $search_box; ?></div>
      <?php endif; ?>
      <div id="header-region">
        <div class="region-inner">
          <?php print $header; ?>
        </div> <!-- /region-inner -->
      </div> <!-- /header-region -->
    </div> <!-- /header -->

Second, the footer. Similar comments apply.

Current footer area:

    <?php if ($footer_message || $footer): ?>
    <div id="footer-region">
      <div class="region-inner">
        <?php print $footer_message; ?>
        <?php if (!empty($footer)): print $footer; endif; ?>
      </div> <!-- /region-inner -->
    </div> <!-- /footer-region -->
    <?php endif; ?>

My suggested footer area:

    <?php if ($footer_message || $footer): ?>
    <div id="footer">
        <?php print $footer_message; ?>
        <?php if (!empty($footer)): ?>
          <div id="footer-region">
            <div class="region-inner">
              <?php print $footer; ?>
            </div> <!-- /region-inner -->
          </div> <!-- /footer-region -->
        <?php endif; ?>
    </div> <!-- /footer -->
    <?php endif; ?>

Perhaps .region-inner isn't the best solution for regions. After all, the entire header isn't really a region -- it's contains a region, but it also contains the logo, site name, and slogan. The ".THING-inner" naming convention works for nodes and blocks. Headers and footers are more like "areas" than "regions" as Drupal defines them.

EclipseGc’s picture

Todd Nienkerk,

Your points are well taken, hmmm... I'm trying to think through all the ramifications of this, how about this as a compromise:

You're right, header and footer are more than just a simple region, we had to bend for content, so what if we do a:

<div id="header-region">
      <?php if (!empty($logo)): ?>
        <a href="<?php print $front_page; ?>" title="<?php print t('Home'); ?>" rel="home" id="logo">
          <img src="<?php print $logo; ?>" alt="<?php print t('Home'); ?>" />
        </a>
      <?php endif; ?>
      <?php if (!empty($site_name)): ?>
        <h1 id="site-name">
          <a href="<?php print $front_page ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>
        </h1>
      <?php endif; ?>
      <?php if (!empty($site_slogan)): ?>
        <div id="site-slogan"><?php print $site_slogan; ?></div>
      <?php endif; ?>
      <?php if (!empty($search_box)): ?>
        <div id="search-box"><?php print $search_box; ?></div>
      <?php endif; ?>
      <div class="region-inner">
      <?php print $header; ?>
      </div> <!-- /region-inner -->
    </div> <!-- /header-region -->

In this way we don't have any extra divs (two sets around just $header seems unnecessary) and we keep a static naming convention. In addition all elements in the header and footer have unique ids on them any way allowing easy altering. In addition to this, we've nailed down that the region-inner (everywhere but content at the moment) is directly before and after the actual print $region.

Footer would be very similar.

I'm not SEO knowledgable enough to +1 the <h1> change, however, it does seem better to me that this would be on the page title, and not the name of the site seeing as that's going to get some good SEO from the <title> element in the header already.

Thoughts?

Eclipse

EclipseGc’s picture

Also, @geerlingguy,

I would not support a page-wrapper because I feel the number of sites that would need such a thing is smaller than the market we're targeting. This is what template over-rides are for.

As for content first, I'm all +1 there, I just am trying not to introduce too many changes at once as we seem to be nailing down the naming/structure etc still, and I'd like to make sure that's all agreed upon so we have as little to nitpick as possible.

Todd Nienkerk’s picture

@EclipseGc: I definitely think we're getting there! I suppose I want to take a step back and ask: What would .region-inner achieve? Is there common styling or placement attributes that would be used across all regions? My gut reaction is no. My gut also tells me that anything ID'd as #foo-region should contain only region output. (My gut says a lot of things!)

I think it would be useful to discuss rationales for .region-inner and #foo-region. What do you find useful about .region-inner and wrapping the header and footer in "region" IDs?

Regarding the #site-name h1: I think we should change this to a div or span. Admittedly, there is some contention about whether search engines really prefer only one h1 per document. Search marketing professions (like my wife) feel very strongly that only one h1 should be allowed; W3C documentation, however, is vague on the subject.

Here's an interesting post that discusses the h1 debate and links to some other resources: Headings and Hierarchy: is More Than One H1 a Crowd?.

EclipseGc’s picture

Todd,

Real quick like, the reasoning for region-inner is that, specifically in 100% layouts we need inner divs to make margins between regions. Header and footer are not excluded from this.

I'm cool with changing the h1#site-name to a div. I like that a lot personally.

Eclipse

Todd Nienkerk’s picture

@EclipseGc: I absolutely understand -- and agree with -- the need for inner divs inside the left, right, and content regions as well as the header and footer. I'm just not sure that *region*-inner is the best way to do that. It feels like we're calling the header and footer "regions" just because, due to a lack of options, we're putting a *region*-inner div inside them (node-inner or block-inner certainly not appropriate here). There are regions inside header and footer, of course, but they are not entirely regions.

What do you think about making .header-inner and .footer-inner divs and leaving the "region" terminology to the divs surrounding actual region output?

EclipseGc’s picture

Well, header and footer ARE still regions. And using a unified class naming structure allows us to apply these things globally. We can still treat header and footer uniquely via #header-region .region-inner {} etc... so I don't see a good reason to deviate from the proposed naming structure unless you can justify what it buys us in flexibility beyond the current proposition. *shrug* Is there something specific this buys us that I'm not seeing? or are you just dissatisfied with the current names?

Let me know,

Eclipse

geerlingguy’s picture

@Todd Nienkerk: I agree with EclipseGc in post #34 - I think it would be less confusing to have all the inner 'regions' be "region-inner," as I, too, consider the header and footer to be 'regions,' just like the others. They're kind of like the roof and foundation on a house, which, imo, are 'regions' still...

JohnAlbin’s picture

I moved the "clear-block to clearfix" change to a new issue because it touches a lot of files all by itself. #371231: Rename clear-block CSS class to clearfix

EclipseGc’s picture

Tomorrow morning I'm really going to try to re-roll another page template with all we've stated here. Really Really.

JohnAlbin’s picture

I promised webchick to post my wishlist/proposal for the page.tpl.php. Finally got time this week to do it.

  1. <h1 id="site-name"> You can use multple H1s on a page, but using an H1 on the site name is a waste of our SEO juice. The only page where an h1 for the site name might make sense is on the homepage. Over in the Zen theme issue queue, we discussed this and came up with code that only wraps the site name with an H1 after checking if $title (the content title) is empty. Also, interestingly, this same question is one that has been debated in the CSS community. http://h1debate.com And it appears to have been decided 2-to-1 in favor of no H1 around the site name/logo.
  2. The repeated use of !empty(). Why do we have these? All of the page.tpl.php's variables are being set inside template_preprocess_page(). Wrapping all these in empty is just noise when a simple if ($varname): works just as well.
  3. Related to the previous note, why do we have <?php if (!empty($messages)): print $messages; endif; ?>? The if statements in the tpl are used to prevent an empty wrapper div from being printed. If the variable is empty and doesn't have a wrapper div, printing it is the same as not printing it. So, again, this is clutter. Just replace those with <?php print $messages; ?>
  4. #container, #main, #main-squeeze, #content, #content-content, what? This nest of divs is poorly named and arbitrarily nested.
  5. Content first. For sure. Non-content-first themes/layout methods can override our default page.tpl.php.
  6. $messages before $tabs. Why bury important messages? Move them just below the $title, so we can see them.
  7. $secondary_menu: There are no themes in D6 that get this right because $secondary_menu is TOO flexible. It can be either the second-level of links from the main menu or a completely separate menu. By default it is configured to be a completely separate menu. And most websites stick separate, secondary links in the footer or as itty-bitty links at the top of the header. I say we stick them in the footer.
  8. $closure: This is a bunch of javascript/debugging stuff. Is there a reason we need it inside the #page div? Let's just stick it before the closing body tag so that it doesn't screw up any #page styling.
Todd Nienkerk’s picture

@JohnAlbin: Your list in #38 is excellent. You've pointed out several things I hadn't even considered but now seem commonsense.

I'm not sure I completely agree with bullet #7, though. I've witnessed first-hand how many people don't realize the secondary menu can be contextual -- that is, is can display different child menus depending on the active parent page -- so know it's often used as a second menu instead of a secondary one. But is this a lack of demand for the feature, or is it simply a lack of awareness? Would more people use the powerful secondary feature if they knew about it -- or if it were named differently? If so, I'd think it would be better to stack them in page.tpl.

EclipseGc’s picture

John,

I'm very on-board for all of those points with the exception that I want to see messages and breadcrumbs moves above the title, outside of the main content div. I'm mentioned that before in this same post, and most people seemed cool with it. Are you find with that?

Eclipse

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new7.72 KB
Failed: Failed to apply patch.
[ View ]

Ok. Here's a patch that does everything I mentioned in #38 and many other items mentioned above.

Things it doesn't do:

  1. The main wrapper goes around content, navigation, left and right divs. Which works great for Zen layout method, but probably not so well for other layout methods. So we may want to move the navigation div above the main wrapper (just below the header). BTW, I haven't seen Zen's neat navigation-after-content-in-html-but-above-visually thingie in any other layout method, so while this is an awesome SEO feat, making it the default html order in Drupal might be a bit too much for people to handle.
  2. Doesn't move $breadcrumb and $mission out of the content div. Where would we put it? Inside the navigation div (if we moved it per the previous comment)?
  3. Doesn't move the $messages out of the content div. According to the Garland-based eye tracking graph at the UofMN usability testing, the eyes started looking at the page directly at the content's $title and then moved down. And then went wandering around to other items. So I feel that putting the $messages directly under the $title would give it the most visibility. (Also, #193482: Styling status messages in system.css)

Also, regarding the #page wrapper. I think 99% of sites are fixed width, centered sites. And the easiest way to do cool styling on it is with a page wrapper div. So I'm a +1 on geelingguy's #5 comment.

@Todd Nienkerk: I think a lack of awareness is a big problem with the secondary menu. However, secondary-as-separate is the default (as opposed to what D5 $secondary_links meant). The biggest problem isn't awareness, its that main and secondary links are way too limiting; they're not configurable enough to show 3+ levels of navigation. Until I re-write Menu Block (#342727: Optimize tree building code) and then get parts of it into core, we're stuck with what we have. And I think that the secondary links are most useful as a separate menu in the footer. And it would be hard for the default page.tpl.php to check the menu settings to see how the secondary links were used and then place them differently based on context. :-p

geerlingguy’s picture

@ JohnAlbin:

Just three points:

1. I think that having the navigation before content makes sense; most sites have it that way anyways, and IMO this affects SEO much, much less than having one or both sidebars before the content. Having the nav links front and center can be a benefit for SEO, on some sites. From a designer's perspective, I like having navigation come structurally before the content.

2. I like $breadcrumb and $mission where they are. I haven't done a site yet where I've moved the breadcrumb to a different area (just above content title, below navigation/header), and I've only had a couple cases where I moved the mission statement into a sidebar (which can be quickly/easily accomplished with a block).

3. I kind of like the idea of messages after tabs... but could go either way on the issue. Having tabs at the top, in some ways, seems more logical; if you were using a file folder, the tab is always above everything that's inside...

Just throwing that out there. Otherwise I'm liking your patch!

(Prays for the day when node.tpl.php is finished, so I can get cranking on a CSS-only theme!).

P.S. I'm all for a #wrapper div. I LOVE styling with a wrapper, and I haven't done a fluid-width site since three years ago (had bad experiences... I'm a designer, not a programmer, and I like my designs to be as I intended them to be!). I would vote for the name #page-wrapper... because that's what it would be doing. (Also, a wrapper can help fluid layouts by allowing the designer to set a max-width and min-width on the whole page...).

JohnAlbin’s picture

StatusFileSize
new10.98 KB
Failed: Failed to apply patch.
[ View ]

I kind of like the idea of messages after tabs... but could go either way on the issue. Having tabs at the top, in some ways, seems more logical; if you were using a file folder, the tab is always above everything that's inside...

Except that messages aren't the contents of the tab. For example, if you go to the Add block tab and submit the form, it brings you back to the List tab and displays a message about the form you just submitted. Obviously, that message isn't related to the current tab.

Here's a new patch. It simplifies some of the divs inside #content, updates the comments at the top, and updates the Stark layout.css to accommodate the content-first order (using a simplified, non-bullet-proof version of the Zen layout method.)

eaton’s picture

I just took it for a spin in stark, and I think the new page structure is refreshingly consistent. Agreed that the 'navigation visually above but structurally below' might be a bit voodooish for most.

The only other question is why there isn't a #navigation-inner -- is it because navigation isn't a proper region?

I'm really, really liking it.

babbage’s picture

Subscribing.

geerlingguy’s picture

Just tested it. I like it a lot! I can't find any real problems with it. I have some fun ideas that I'll be working on once we finish node.tpl.php :-)

JohnAlbin’s picture

The only other question is why there isn't a #navigation-inner -- is it because navigation isn't a proper region?

Actually, yes. I originally had a <div id="navigation-inner" class="region-inner"> in there, but the "region-inner" class was just wrong.

So, we have 3 choices on that point:

  1. do NOT include an inner div like <div id="navigation-inner" class="region-inner">
  2. include an inner div like <div id="navigation-inner"> but with no "region-inner" class
  3. Add a $navigation region and include an inner div like <div id="navigation-inner" class="region-inner">

The Zen theme actually uses a #3 approach. But that also opens the floor to this question: What (if any) additional regions do we want in the default page.tpl.php?

webchick’s picture

What (if any) additional regions do we want in the default page.tpl.php?

"above content" plzkthxbi.

The "Content" region is about the most confusing frigging thing in the world.

geerlingguy’s picture

@ webchick: I don't know why I didn't think of that, but yes, an 'above content' region would be immensely useful. Especially if one of the objectives of this ordeal is to make it so advanced theme designs can be solely CSS; a lot of sites I work on end up having something or another above the content section.

JohnAlbin’s picture

"above content" region++ :-)

We should also rename the "content" region to be "below content". In addition, I think we should make that a separate variable from $content so that it is similar to how $footer and $footer_message got split out in D6. $content_above and $content_below variable names? or $above_content and $below_content variables?

any others? navigation region? closure region? nether region?

catch’s picture

content_top and content_bottom?

geerlingguy’s picture

I like content_top and content_bottom. I don't know about any others, though... If someone wants to go region-crazy, they can make their own template files. While we want to make the theming able to be quite advanced, we don't want to overwhelm newcomers with too many regions or options.

babbage’s picture

In CSS, I'd much rather see content-top and content-bottom than content_top and content_bottom. Just my 2c. :)

geerlingguy’s picture

As per #53, yes, I too would like to stick to dashes (-) rather than underscores (_) for CSS. Underscores seem to be used only for PHP normally, so this would seem to be a good idea to me.

webchick’s picture

@dbabbage: That's fine. They can be wrapped in divs with IDs of content-bottom/content-top. But PHP variables (print $content_top / $content_bottom) cannot contain hyphens; only underscores.

EclipseGc’s picture

@dbabbage & @geerlingguy

In fact, if I'm not mistaken, that's our defined standard.

@johnalbin,

I would definitely ++ the inclusion of content_top and content_bottom. But I see no need to add a navigation region. This is hardly ever going to be used, and if someone needs it, the answer is, imo, create a new page template. *shrugs* I'm also not sure I see a point in putting an inner div on it, as the menu output is already pretty heavily namespaced, however, if we want to do that for consistency sake I guess I'm ok with it.

What I am questioning at this point is all the #region-name-inner ids we have. I'm not sure I see the point for them frankly. In fact, we can easily get any of those same divs with:

#header .region-inner {
  css:stuff;
}

Which is actually more specific than #header-inner would be and still very consistent one div to the next. I would really love an explanation for going the current route as opposed to this method (which makes for cleaner html).

Additionally I'd really love to dissect the #logo-title section. I put a bit of thought and effort into this in my attempt above as I really don't like what's there currently, and I think you've mostly copied what's in the existing template (please correct me if I'm wrong). Is there a reason I should end up with an empty #logo-title and #name-and-slogan divs on every page if I don't use those elements? Please advise.

Per our previous discussions I still really don't like breadcrumbs and messages in the content. I think it's completely the wrong place for them (for all the reasons you stated concerning tabs above to geerlingguy). I know you have mentioned usability, and I think this is MORE usable (outside the content area) but we won't know for sure until we get some testing on it. I cannot argue from this angle at all, it's very crippling. What I can do is say this:

My method makes the messages larger, this means they'll stand out better. As you've already mentioned the messages really have little to do with the content, and I think this lends credence to my solution. In the same way breadcrumbs are really a function of the page, and not the content. I really don't like them within the content div, and I really don't think it hurts us any to do so.

From an html perspective, I don't understand how making these their own divs with ids and clear-block classes on them hurts our #main container.

Finally let's talk about header and footer. On the header you've placed a #header, with #header-inner.region-inner along with a #header-region around the $header itself. I think I proposed placing the region-inner div directly around $header and allowing all other header content to sit directly in #header. This allows each container of each content item to do its own placement as necessary. I'm not sold on my own approach here. In fact I'm okay with yours. What I'm wondering is why you didn't do the same thing in the footer?

OK, so that was super nitpicky. hehe sorry all.

Eclipse

wretched sinner - saved by grace’s picture

JohnAlbin’s picture

@EclipseGC: Re: the logo-title divs. I didn't see that change in your patch. That was an unintentional omission. I agree empty divs are annoying. Also, “nitpicky” comments are fine if they are married with “useful”, which yours totally are. :-)

For reference, the last major revision to the page.tpl.php was here: #163723: Fix default page.tpl markup. Funnily enough, they used the (as my co-workers refer to it) “Pre-JohnAlbin version of Zen” as the basis for those changes. :-)

I have a much longer response I need to write, but I won't have time until tonight to write it. I sold all my TVs yesterday, so Drupal will be my sole entertainment tonight.

EclipseGc’s picture

woo, commitment much? yes. lol

Ok cool, I just don't want to come across wrong. On the logo/stuff the only change I'd make to my version of the code is changing the h1 to a div. If you have other thoughts there, fire away.

I look forward to the rest of your response.

Eclipse

yoroy’s picture

Just talked geerlingguy in IRC a bit. Can't help out here, but I'm confident you're all doing good things here :)

I have only one request:

Please, please, please move the $help variable to the bottom of the page.
We have so much blabla keeping people away from the actual functionality (block admin page, anyone?), it's just plain impolite and unhelpful and has been observed being a true usability issue. People don't read manuals, really, they don't. Shoving it them in the face constantly is not the way to handle this.

Even if all those help text get transformed into a little clickable help icon when the new help system gets in, it should still be placed below the actual 'content' of the page: Let me try and figure it out first, if that didn't work, I probably scrolled down to the bottom of the page, hey look: Help…

Thank you!

geerlingguy’s picture

A summary, then, since the patch in #43, along with a couple comments of mine:

1. (#47): I like approach 3, "Add a $navigation region and include an inner div" ... but as per #56, it might not be necessary. I'd like it for consistency's sake, though.

2. (#48-55) Include $content_top and $content_bottom (CSS: .content-top and .content-bottom) regions.

3. (#56) Fix up the #logo/#title section a bit. Make it have conditional PHP statements?

4. (#56) Perhaps move messages either completely out of the $content region, or into the new $content-top region.

5. (#58) JohnAlbin sold all his TVs. That's dedication!

6. (#59) Change the logo/header title to a <div> instead of <h1>.

7. (#60) (Possibly) move the $help section below the main content... and possibly even make it so it's not visible unless the user clicks a 'help' button. (I would be happy with having it be a help button, like a little question mark icon).

webchick’s picture

@yoroy: I think that particular proposal needs a wider discussion (including what this help looks like when it's eventually added), and is out of scope for a relatively simple issue of re-naming and re-ordering some HTML tags. Could you start a thread on the usability group and post back here? Whatever the consensus is, we can then implement it in a follow-up issue.

EclipseGc’s picture

@webchick,

Thanks for catching that before we get mired in it. :-) Definitely a separate issue thing.

@johnalbin,

Dude, where's my reply? I waited all night and got up at 5:00 am! and STILL nothing? ;-)

Eclipse

JohnAlbin’s picture

StatusFileSize
new12.03 KB
Failed: Failed to run tests.
[ View ]

So, one of the fundamental questions about the markup in page.tpl.php is “Who will be using the default page.tpl.php?” I think the answer to that will be people who know CSS, but don’t know how to or don’t want to change the markup. So a super-lite html markup with no wrapper divs won't serve them well. I think we should strive to implement the minimal number of divs that will be necessary to complete a task.

If we look at the logo, site name, slogan section of the page from that perspective, it seems clear to me why we have so much markup. Since the logo will be some unknown size and will often be floated left, we are forced to have a div around site name and slogan; otherwise its difficult to guarantee the slogan will be aligned properly with the site name. So how about we nix the #logo-title div, but leave the #name-and-slogan div?

Re: navigation region: Including a navigation region for the sole reason of having consistency is probably a bad idea. I included a navigation region in Zen because I think the primary links list is pretty limiting and I often need more complex navigation. However, since Drupal core doesn't yet have a way to do more complex navigation (think “Menu block” module in core), I'm fine with that being a separate issue. For now, let's leave navigation the way it appears in #43.

I've included “Content top” ($content_top) and “Content bottom” ($content_bottom) regions. I think I would prefer text descriptions of “Above content” and “Below content”, but I’m unsure what the corresponding php variables should be called if we used those descriptions. Are top/bottom sufficient? If above/below is better, what var names?

But now… we have a serious semantic failure of the .region-inner class. The #navigation div has no region. The #header div contains much more than the header region. The #content div has 2 regions. Even #footer has more than just the footer region in it. Those “things” aren’t regions. Having spent maybe 60 minutes total looking at HTML 5, I see that HTML 5 would define those “things” as Sections. So I propose using .section classes. What say you?

Re: #foo-inner vs. .region-inner: Ok, here's a 90° turn, if we rename .region-inner to .section. How about we get rid of those #*-inner ids that are also sections. That would leave just #page-inner and #main-inner. The one conceptual problem I've seen in Zen is occasionally I'll have people ask “What are all those inner divs? I dont't get it.” And when I say that just how we named our wrapper divs, they go “oh! I understand now.” If you do a google search, I think "wrapper divs" is used 5 times more than "inner divs". So we could also rename the #page, #page-inner to #page-wrapper, #page and #main, #main-inner to #main-wrapper, #main. And that would align the markup with CSS terminology.

@EclipseGc: Dude, I got up early too! Here's a patch to make up for your "lost" hour.

Status:Needs review» Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Whoops!

Notice: Undefined variable: content in include() (line 56 of themes/garland/page.tpl.php).

Crap. The new regions are screwing things up. Looks like the $content variable is not generated if there is no $content region. :-\ I don't have time to look at this now.

Anyways, the page.tpl.php's HTML still needs review and feedback.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new12.91 KB
Failed: 9727 passes, 8 fails, 0 exceptions
[ View ]

This re-rolled patch now renders $content if it's not a region.

Status:Needs review» Needs work

The last submitted patch failed testing.

kika’s picture

H1 discussion over here too: http://groups.drupal.org/node/18956

JohnAlbin’s picture

StatusFileSize
new13.88 KB
Failed: 9758 passes, 10 fails, 0 exceptions
[ View ]

Test failed: ah, bugger. I should have known messing with the regions would cause the block tests to fail.

Now that the #371231: Rename clear-block CSS class to clearfix issue has been committed, here's a re-roll. This also patches the block.test.

JohnAlbin’s picture

Status:Needs work» Needs review

testbot please.

Status:Needs review» Needs work

The last submitted patch failed testing.

ejort’s picture

subscribe

JohnAlbin’s picture

Ok. I just had a very good chat with EclipseGc in IRC where I more fully explained the motivation for the classes and IDs in the last patch. And, I figured, hey, I haven't said enough in this issue already, so… ;-)

I prefer simple IDs for the "section" divs. I've never liked *-wrapper IDs because it makes the positioning rulesets longer. And *-inner IDs are just marginally better. But I also see the necessity for 2 divs around each positioned section, so what are you gonna do? Zen has been using *-inner IDs but that does cause occasional wtf?'s from people until they realize they are just wrapper divs by another name.

EclipseGc made a good argument (in a previous comment above) that <div id="ID"><div id="class"> makes higher specificity rulesets of #ID .class. The only problem with that argument was that we were (at the time) using "inner" as the class name. And its easy to get nested "inner" classes. And then the #ID .inner ruleset becomes problematic. But, IMO, a .section class seems unlikely to result in nested .section classes. And, presto! the original class-only-inner-div solution makes a lot sense.

That still leaves the #page and #main which are pure wrappers and not even page sections. And here I can't think of a solution different than the <div id="ID1"><div id="ID2"> solution. So, I resigned myself to using -wrapper IDs since "wrapper" is used much more than "inner" in the CSS community. And its only in two places.

I'm also fine with putting a "region" class around the immediate div parent of each region. I just noticed I missed a couple of those in the most recent patch.

Tonight, I'll try fixing that block.test failure. And if I can't do it, I'll move "split $content into $content_top and $content_bottom" to its own issue. Then re-roll with the missing "region" classes. And… be done?

tomamic’s picture

Actually, .section might be used in content as an interim solution until html5 will finally introduce full <section> elements. Moreover .section is quite misleading.

http://www.mozilla.org/contribute/writing/markup#section

JohnAlbin’s picture

@tomamic: How is a section class “quite misleading”? We're trying to use .section in the same way HTML5 uses <section>. I don't see how that is misleading.

Anyone know what the purpose is of that Mozilla.org document? Is it a style guide for Mozilla.org documentation? It appears to be an orphan page with an AWOL parent. Is it a failed "let's create and organize some semantic classes" effort?

Putting aside Mozilla.org for a moment, should we be concerned about possible .section classes in content? There won't be any .section classes in any other tpl files.

EclipseGc’s picture

Well, anyone could do anything they want, we're only concerned with the default output, as this should be the model to follow. Your explanations are good. Do everything you said in #74 and I'll review again, and hopefully we'll be there. I look forward to seeing this committed!

Eclipse

geerlingguy’s picture

Agreed with EclipseGc on #74 - and @ JohnAlbin: I like your idea with the sections - following an HTML5 mindset is a step in the right direction, and the .sections sections seem logical to me.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new15.93 KB
Unable to apply patch drupal-page-tpl-367299-79.patch
[ View ]

Ok. Figured out the block.test failure. Simple tests use Garland when running, so I had to update all the core themes that had content regions as well (garland, bluemarine, pushbutton).

I think I have all the IDs and CLASSes correct. But please triple-check; I already double-checked. ;-)

Also, you should eyeball the new regions over on admin/build/block. To reiterate, $content is no longer a region. $content_top and $content_bottom are the regions.

webchick’s picture

Hrm. I'd kind of rather not make this a core-wide change. But if we DO do that, then we need to write an upgrade path as well to move peoples' blocks that were formerly in the content region over to content_bottom.

What's the deal with the failing tests?

JohnAlbin’s picture

StatusFileSize
new14.89 KB
Failed: 9559 passes, 189 fails, 8319 exceptions
[ View ]

The failing block tests were because I assumed Simpletest was using the default page.tpl.php, buts its using Garland. And after I modified the default regions in system_theme_default() there was a region mis-match between the default and what Garland was expecting. I know what's going on now, so I can make sure the block.tests don't fail no matter what direction we take.

And, if you don't want it to be a core-wide change, I will revert the changes I made to those other page.tpl.php and modify their .info files so that they continue to use their old regions. That would eliminate the need for an upgrade path for blocks.

New patch does this.

webchick’s picture

Well, I'm open to discussion/debate. I think content_top/content_bottom is a good improvement, just I envisioned making any kind of sweeping changes to other themes/template files in a separate issue, *after* we get the default markup of page.tpl.php solved. It's also not clear to me why we'd want to change Garland's default regions, but not its default containing divs and stuff to match what we're doing here.

Status:Needs review» Needs work

The last submitted patch failed testing.

EclipseGc’s picture

doesn't this mean the test should be altered so that it DOESN'T use garland? I haven't done much testing here, so I can't speak to it much, but it seems like the test should run against either the current theme, or ALL themes individually... no? In any event this seems like a separate issue to me. I say we scrap content_top/bottom for the moment, and push that into a separate issue so we can speed along this commit. I'll check the code out today and get a review up (I'll use the working patch for the moment as I doubt the markup has changed much).

Let me know what we're thinking here. I think we just dove into territory I wanted to avoid.

Eclipse

webchick’s picture

No, the test tests the default installation profile, which means it will use garland, unless we want all Drupal sites out there to ship with stark by default. ;)

As discussed in IRC, we'll move the content region discussion to a separate issue (i believe there is one already).

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new11.21 KB
Unable to apply patch drupal-page-tpl-367299-86_0.patch
[ View ]

Wow! I really broke stuff. I was just running the block test on my system before and they all passed. Now I'm getting: 189 fails, 8319 exceptions. :-p

Ok, agreed. Lets move "split $content into 2 separate regions" to a follow-up issue; its causing too many cascading issues for this patch. We can add the markup for those regions’ divs into page.tpl.php in that issue. Its only 3 lines of HTML anyway.

geerlingguy’s picture

Re: #86 - Love it, love it, love it!

I'm happy with the way things are now. Will go over it with a more fine-toothed comb later this weekend, but from a quick run-through, it looks mighty fine to me.

The content_top/content_bottom thing would be nice to get, so hopefully the other thread will result in a spiffy patch/upgrade.

JohnAlbin’s picture

Here's some follow-up issues to pursue after this issue is resolved.

#378916: Split content region into content_above and content_below
#378948: Make left and right sidebars more RTL friendly

They might be duplicates since Advanced Search isn't working on the issue queue right now. :-p

geerlingguy’s picture

StatusFileSize
new7.58 KB

To any designers interested in diving in to the #86 patch without having to get their hands gooky in the patching process... attached is a patched page.tpl.php file (NOTE: If you're using the Stark theme with this modified page.tpl.php file, some of the layout will be changed... but at least you can see what the new template file does).

To use the file:

  1. Download the attached file.
  2. Rename the downloaded file to page.tpl.php.
  3. Copy this file into /modules/system in the root of your drupal 7 installation.
EclipseGc’s picture

Status:Needs review» Needs work

OK, this is looking REALLY good and very close. I've marked this as CNW because the block & menu pages fails in IE6. (something that doesn't happen with the current output) If I have a chance I'll try to chase this bug down, but nothing's popping out at me yet.

.section is currently padding, and I think I'd prefer to see that as margin. Any reason you went with padding?

My one serious question is this:

    <?php if ($main_menu): ?>
      <div id="navigation"><div class="section">
        <div id="main-menu" class="clearfix">
          <?php print theme('links', $main_menu, array('class' => 'links main-menu')); ?>
        </div> <!-- /#main-menu -->
      </div> <!-- /.section, /#navigation -->
    <?php endif; ?>

Why 3 divs around this? If we had primary and secondary it'd make sense, but we don't, so wouldn't this be better as:

    <?php if ($main_menu): ?>
      <div id="navigation"><div id="main-menu" class="section clearfix">
        <?php print theme('links', $main_menu, array('class' => 'links main-menu')); ?>
      </div></div> <!-- /.section, /#navigation -->
    <?php endif; ?>

Also, only appears to be 2 closing divs for 3 opens, which might be our problem in IE6 (now that I'm looking).

Can we walk through the rationale for moving secondary nav out of here? I'm fine with it, I just want to have it clearly stated cause I think I missed it.

So, this stuff aside, I'm REALLY a big +100 on where this is going. Maybe a little more conversation and 1 more revision?

Eclipse

JohnAlbin’s picture

StatusFileSize
new11.02 KB
Unable to apply patch drupal-page-tpl-367299-91.patch
[ View ]

.section is currently padding, and I think I'd prefer to see that as margin.

Yay! A bikeshed. Let’s compromise by putting padding on the left and top and margin on the right and bottom. k? ;-)

The rationale for moving secondary menu into the footer was:

  1. Moving it was in my initial to-do list in #38 above (point 7).
  2. Then Todd Nienkerk brought up a good point in #39
  3. and I responded in #41

And that was the sum total of the discussion.

Also, only appears to be 2 closing divs for 3 opens

Can haz validz HTML? FAIL.

Ok, this new patch just changes the stuff around the main and secondary links. Rather than have a with ID wrapped around the menu, I simply applied the ID directly to the theme('links') call. We were already applying classes to that function, so I don't see any reason not to add the ID as well. This also eliminates the need for the <?php if ($secondary_menu): ?> since theme_links won't return anything if its given zero links.

geerlingguy’s picture

@ John Albin RE: The secondary menu thing...

Typically, I just use the secondary menu as another set of links... however, there are projects where I like putting it just below the primary and using it as a second level primary links block. I didn't even know how to do this until a week or two ago, because documentation of the secondary links menu is poor at best.

Whatever the case, maybe we could make a point to document this change somewhere in the handbook, and explain to themers how to take control of their secondary links? (I don't really know where, because I haven't even seen a page on primary/secondary links in the Drupal 6 handbook...).

JohnAlbin’s picture

Status:Needs work» Needs review

Review!

geerlingguy’s picture

My stamp of approval is given. Can't find something to hate about the markup, either PHP or HTML. I've looked through it three times now; the only thing I didn't do is put it up on a live site and validate it, or test in IE 6. Maybe EclipseGc can do that ;-)

(As an aside, I have chosen this year (a "New Year's Resolution") to stop developing for IE 6. I'll only fix major bugs, but not minor layout problems. If one of my clients has a problem with that, well fuggedaboutit!).

EclipseGc’s picture

StatusFileSize
new11.04 KB
Passed: 10268 passes, 0 fails, 0 exceptions
[ View ]

OK John, I don't think this padding approach for the content first layout is going to work. It's great in FF, and IE7 (tested both) but it fails in IE6 when paired with tabledrag components. I don't know why this is, but if we resort to using left placements it works across the boards, FF, IE7, IE6, Safari and Chrome (i've tested all of these with my new code). I also switched things to margin as I think we're misusing padding right now.

In the case of background colors that need to be separate from the container, padding isn't going to buy us anything as the bgcolor/image will extend out through the padding. Padding's mechanism is to push text in within a div, margin's is to push in the borders of a div, which is clearly what we're doing on the inner divs. If we needed the former a second div would not be required as we could simply state some padding on the first div in the stack. I'll get use cases together if I have to here :-D.

That said, the only things that has changed on this one is the css, I think John's currently proposed html is major ++ from me, we just need to get the css right (which I hope I've done here). Test it out and let me know!

Eclipse

EclipseGc’s picture

StatusFileSize
new11.16 KB
Unable to apply patch page_patch_367299_2_1.patch
[ View ]

After conversations in IRC with JohnAlbin about some mistakes in the previous patch I've rerolled and re-documented the css to properly reflect the changes I made. IE6 still has issues, but they're on what I consider to be acceptable edge cases thus far. (Over-large images in the sidebars)

No changes from the previous patch except code standard/documentation fixes.

eaton’s picture

Just installed it and poked around: I really, really like this.

It gets my thumbs up; unless JohnAlbin has objections to it, I know *I* would be thrilled. ;-)

JohnAlbin’s picture

Status:Needs review» Reviewed & tested by the community

Ok. geerlingguy and Eaton have thumbs up'ed it. In IRC, EclipseGC RTBC'ed the part of the patch I rolled. And I'm RTBC'ing the part of the patch he rolled.

I think we're good!

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Commmmmmmmmmmmmiiittttttteeeeeeeeeeeddddddd! Yay!! :D

Thanks so much for all your hard work, folks! :D

Please ping back here with follow-up issues.

EclipseGc’s picture

My bad!

eigentor’s picture

Wow this is great. Pure CSS Themes and standards-compliant, neat and clean...
Well actually this is a disguised subsribe post. Hope you forgive me.

tstoeckler’s picture

Status:Needs review» Fixed
webchick’s picture

kika’s picture

Issue tags:+tpl-refresh

adding a tag to group tpl stuff

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

loopy1492’s picture

Hello. I see there are patches here, but my download of Stark didn't come with a page.tpl.php file in it. Is there a simple download I can use to place in my modified Stark theme? I'd like to move my breadcrumbs somewhere else without having to use jQuery to do it.

Edit: never mind. I figured out where the default one was (in /modules/system/).