Both footer regions contain unnecessary nested region <div>'s. We should get rid of them. I don't believe they are needed, but if for whatever reason they are, we should adapt region.tpl.php to include the markup needed. There is also a "sitemap" class on each footer column region which is pretty odd.

Patch coming...

CommentFileSizeAuthor
#14 bartik-nested-region-classes_3.patch5.69 KBJeff Burnz
#9 bartik-nested-region-classes_2.patch5.93 KBJeff Burnz
#1 nested-region-classes.patch4.6 KBjacine

Comments

jacine’s picture

jacine
she/her
Location New York
commented
Status: Active » Needs review
StatusFileSize
new nested-region-classes.patch4.6 KB
Jeff Burnz’s picture

Jeff Burnz commented

+1, thinking for consistency this should be rolled out to all regions, sidebars have wrappers also and we have this rather redundant "section" wrapper on nearly everything (clearly header and footer need a constraining DIV). Other than that most regions and other page output have triple wrappers which at a glance could be done with one.

jensimmons’s picture

jensimmons commented

So stuff like this:
<div id="featured" class="region"><div class="section clearfix">
is in Bartik because that's how it's written in the core files.

div id
followed by
div class section

replaces
div id
followed by
div example-inner class
that's all over D6. In Zen, in other many themes too.

Why does Drupal provide 2 divs, nested inside of each other by default — because of IE, and it's inability to understand the CSS boxmodel properly. Having 2 divs like this let's people easily write much better CSS.

I like the way the 2 nested divs have been renamed, so I used the new convention. (And even if I didn't like it, I'd use it anyway to be consistent and Druply.)

It might be true that it's not as needed anymore. New techniques have come along. IE 8 is better than IE6... but I hesitate to remove them because: it's the Drupal way to have them, and frequently they are still needed.

[I know there is not CSS in Baritk using these (or using most of them.) But Bartik has been created to be flexible for people who are using it as a base theme. I know people should use other themes as a base theme... I know Bartik is not going to be all things to all people, but I still feel passionately that Bartik should work for any front-end developer who is new to Drupal and uses Bartik as the base for their first custom Drupal site. Because that's what everyone does.]

So, back to the main thing at hand.... do we take these out.

jacine’s picture

jacine
she/her
Location New York
commented

[I know there is not CSS in Baritk using these (or using most of them.) But Bartik has been created to be flexible for people who are using it as a base theme. I know people should use other themes as a base theme... I know Bartik is not going to be all things to all people, but I still feel passionately that Bartik should work for any front-end developer who is new to Drupal and uses Bartik as the base for their first custom Drupal site. Because that's what everyone does.]

Who is talking about a base theme? I'm not. I am and have been totally on board with your thinking on how it should be easier for people to front-end devs use. This is something I have spent a lot of my personal time working towards in this community, so I'm not sure why you are responding defensively with this here... I'm saying the way it is right now is not ideal and is potentially confusing. As a front-end developer that has been theming Drupal sites for about 4 years, when I realized these duplicate classes were there, it confused me for a good 10 minutes.

Here's an example of what I'm talking about:

Before:

<div id="triptych-wrapper">
  <div id="triptych" class="clearfix">
    <div id="triptych-first" class="region triptych">
      <div class="section">
        <div class="region region-triptych-first">...</div>
      </div>
    </div><!-- /.section, /#triptych-first -->  
    <div id="triptych-middle" class="region triptych">
      <div class="section">
        <div class="region region-triptych-middle">...</div>
      </div>
    </div><!-- /.section, /#triptych-middle -->
    <div id="triptych-last" class="region triptych">
      <div class="section">
        <div class="region region-triptych-last">...</div>
      </div>
    </div> <!-- /.section, /#triptych-last -->
  </div>
</div> <!-- /#triptych, /#triptych-wrapper -->
<div id="footer-wrapper">
  <div class="section">
    <div id="footer-columns" class="clearfix">
      <div id="footer-firstcolumn" class="region sitemap">
        <div class="section">
          <div class="region region-footer-firstcolumn">...</div>
        </div>
      </div> <!-- /.section, /#footer-firstcolumn -->
      <div id="footer-secondcolumn" class="region sitemap">
        <div class="section">
          <div class="region region-footer-secondcolumn">...</div>
        </div>
      </div> <!-- /.section, /#footer-secondcolumn -->
      <div id="footer-thirdcolumn" class="region sitemap">
        <div class="section">
          <div class="region region-footer-thirdcolumn">...</div>
        </div>
      </div> <!-- /.section, /#footer-thirdcolumn -->
      <div id="footer-fourthcolumn" class="region sitemap">
        <div class="section">
          <div class="region region-footer-fourthcolumn">...</div>
        </div>
      </div> <!-- /.section, /#footer-fourthcolumn -->
    </div><!-- /#footer-columns -->
    <div id="footer" class="clearfix">...</div><!-- /#footer -->
  </div>
</div> <!-- /.section, /#footer-wrapper -->

Each region has 2 .region and 2 .section classes being applied to it. That makes no sense.

Proposed After:

<div id="triptych-wrapper">
 <div class="section">
    <div id="triptych" class="clearfix">
      <div class="region region-triptych-first">...</div>
      <div class="region region-triptych-middle">...</div>
      <div class="region region-triptych-last">...</div>
    </div>
  </div>
</div><!-- /#triptych, /#triptych-wrapper -->

<div id="footer-wrapper">
  <div class="section">
    <div id="footer-columns" class="clearfix">
      <div class="region region-footer-firstcolumn">...</div>
      <div class="region region-footer-secondcolumn">...</div>
      <div class="region region-footer-thirdcolumn">...</div>
      <div class="region region-footer-fourthcolumn">...</div>
    </div><!-- /#footer-columns -->
    <div id="footer" class="clearfix">...</div><!-- /#footer -->
  </div>
</div><!-- /.section, /#footer-wrapper -->

Still plenty of divs... I'm not talking about removing inner divs that are needed for box model stuff. There are more than 2 nested DIV's when the code in region.tpl.php is combined with what is in page.tpl.php. If you want to keep 3 nested DIV's that's your decision, but at least get rid of nested region and the odd sitemap classes...

It might be true that it's not as needed anymore. New techniques have come along. IE 8 is better than IE6... but I hesitate to remove them because: it's the Drupal way to have them, and frequently they are still needed.

The Drupal way, is that themes are allowed to do what they want with their markup... If you want to keep the .section div inside each region, that's your choice, but I think it should be done in region.tpl.php. ID's can be added very easily, and even the code comments if you wanted them. IMO this is easier for newbies to understand. If you don't want to go that route, then you could also just print the region content itself in region.tpl.php and put all the wrappers in page.tpl.php. It's the fact that 1/2 the region code is printing in page.tpl.php and the other 1/2 that is printing in region.tpl.php is providing some of the same classes that is a problem.

Hopefully I am making sense and you just misunderstood?

BTW, Zen is using the region template files and is not giving regions multiple nested .region and .section classes.

Jeff Burnz’s picture

Jeff Burnz commented

One of the aims I believe of the region wrapper and new $classes variables is bring more consistency to Drupals markup. I think if there's any drive towards a "Drupal way" its to remove markup from page.tpl.php and abstract it to the other templates - most notably region and block templates, so page.tpl.php only includes very basic markup, with all the design hooks going into the nested templates.

I think this is conceptually right in the new paradigm of Drupal templates - region.tpl.php has suggestions and we can use those to good effect. I see this is how Zen has approached it in D7 and I agree very much with this approach (with its region--sidebar.tpl.php).

On a more personal level I like lean efficient markup - I think its good for the web, good for the website - every single page speed tester tells you to remove unneccessary DOM elements - why? Because pages load faster, render faster, execute JS faster and so on - especially for low power, low bandwidth devices (mobile web).

http://developer.yahoo.com/performance/rules.html#min_dom

jensimmons’s picture

jensimmons commented

Oh, I wrote this up the other day on the bus, and never submitted it:

Yeah, sorry if I came across defense. That was dumb of me. No one in this queue has sad anything besides awesome things.

And mostly, I wasn't sure at all about it. About if there are too many divs. LOL. I'm sitting here laughing at any idea that I'm saying "we need those divs! They are the Drupal way, so let's have tons of divs."

Uh, did I say that. :(

Thanks for writing out the code — it's easy to see there what you mean. Yeah, that's way too many divs. Let's clean it up just like you suggest. I'll take a deeper look once I'm back in NYC.

jensimmons’s picture

jensimmons commented
Title: Nested Region DIV's in Featured, Footer and Triptych code » Simplify footer region divs's in featured, footer and triptych
Project: Bartik » Drupal core
Version: 7.x-1.x-dev » 7.x-dev
Component: Code » Bartik theme
Priority: Normal » Minor

Status: Needs review » Needs work

The last submitted patch, nested-region-classes.patch, failed testing.

Jeff Burnz’s picture

Jeff Burnz commented
Priority: Minor » Normal
Status: Needs work » Needs review
StatusFileSize
new bartik-nested-region-classes_2.patch5.93 KB

re-roll

Jeff Burnz’s picture

Jeff Burnz commented

relates to #883408: Remove redundant section classes and wrapper divs from bartik

dries’s picture

dries commented

Looks much cleaner to me, and I'm not even a designer. Would be good to get sign-off from another designer.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Much cleaner and simpler, and less confusing. Awesome!

jacine’s picture

jacine
she/her
Location New York
commented
Status: Reviewed & tested by the community » Needs work

Yay, Thank you Jeff! You rock. :D

Just one nit-pick...

+++ themes/bartik/templates/page.tpl.php	14 Aug 2010 22:18:38 -0000
@@ -229,31 +213,10 @@
+        <?php print render($page['footer_firstcolumn']); ?> <!-- /.region-footer-firstcolumn -->
+        <?php print render($page['footer_secondcolumn']); ?> <!-- /.region-footer-secondcolumn -->
+        <?php print render($page['footer_thirdcolumn']); ?> <!-- /.region-footer-thirdcolumn -->
+        <?php print render($page['footer_fourthcolumn']); ?> <!-- /.region-footer-fourthcolumn -->

Can we check if the region is populated before printing these comments, or even cleaner - move them to region.tpl.php?

Powered by Dreditor.

Jeff Burnz’s picture

Jeff Burnz commented
Status: Needs work » Needs review
StatusFileSize
new bartik-nested-region-classes_3.patch5.69 KB

Maybe we just don't need those comments, looking at the reset of page.tpl.php it seems it would be more consistent to not have them.

jacine’s picture

jacine
she/her
Location New York
commented

That's fine with me. I think this is RTBC, but I'll wait a few in case anyone else has any thoughts on this... :)

Bojhan’s picture

Bojhan commented
Status: Needs review » Reviewed & tested by the community

Ok, RTBC then - we have waited a few :)

bleen’s picture

bleen commented

*much* more gooder

RTBC++

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

Tjarco’s picture

Tjarco commented
Assigned: Unassigned » Tjarco
Category: task » support
Status: Closed (fixed) » Postponed (maintainer needs more info)

Dear Jeff & others,

I'm really new to Drupal, started today with creating a site with the bartik theme.
The tryptich normally contains 3 regions (first, middle & last) i want to customize this into one region
is this possible with the bartik-nested-region-classes_3.patch?

any help on how to implement would be welcome,
I would have started, downloading the mentioned CSS files and manually changing the codes as provided

will this result in just one tryptich region?

many thanks in advance!

Tjarco
Drupal & Scripting N00b

bleen’s picture

bleen commented
Assigned: Tjarco » Unassigned
Category: support » task
Status: Postponed (maintainer needs more info) » Closed (fixed)

Tjarco - welcome ...

first, I suggest that you simply dont use "middle" and "last" .... if your just playing (to learn your way around) then you can easily make the tryptic region into one region by simply editing the css. If you are planning on using bartik for a real site, then I suggest you create a "subtheme" (google "drupal subtheme for tons of docs) in order to slightly modify Bartik

Second, you shouldnt change the various meta data fields on an issue like this (especially the "assigned" field) unless you have good reason pertaining to the nature of the issue.

In this case you probably would have been better off opening a new support ticket. Also, as you are a beginner I HIGHLY recommend going to the drupal channels on IRC (http://drupal.org/irc) for questions.