Use clearfix instead of <div style="clear:both"></div> in page.tpl.php where applicable [#1047388]

Attached is a patch that changes the use of "

" to class="clearfix" in page.tpl.php where applicable.

The use of clearfix is now a Drupal standard and is a part of Drupal core: http://drupal.org/node/254940#clearfix

You are already using clearfix in node.tpl.php so I assume you are comfortable / familiar with this approach.

There were two instances of "

" that I left in the page.tpl.php in this patch because the clearfix solution wouldn't necessarily apply.

And thanks for this great-looking theme! It came in really handy for us in a pinch.

Comment	File	Size	Author
#4	danland-1047388-3-whitespace.patch	17.5 KB	tim.plunkett
#4
#4	danland-1047388-3.patch	5.35 KB	tim.plunkett
#4
	danland-clearfix.patch	17.22 KB	acouch

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

danpros CreditAttribution: danpros commented 1 February 2011 at 16:18

Version:	7.x-1.0-rc2	» 7.x-1.0
Category:	bug	» task

Hi,

Thanks for the patch. But its different.. I use global fix for this, the D6 is using <div class="clear-block">, D7 using <div class="clearfix"> but I am using <div style="clear:both"></div>. That will always work in any Drupal version since this is CSS standard.

Dan,

Comment #2

acouch CreditAttribution: acouch commented 1 February 2011 at 16:51

hi Dan,

Using <div class="clearfix"> is considered a best practice because it doesn't use the extra markup that using <div style="clear:both"></div> does. '

' is not really a CSS solution like clearfix is because it requires extra markup.

All of the places I removed the <div style="clear:both"></div> are instances where the 'clearfix' is added to an already existing div.

Your theme is visually beautiful and I would like to help make the template match!

Comment #3

danpros CreditAttribution: danpros commented 1 February 2011 at 17:31

Status:

Active

» Patch (to be ported)

Hi Aaron,

Sorry for fast replay but without thinking it first, I have a lot of issue queue and I hate ignoring people, so sometimes my answer will look like silly :)

You are right it will add an extra markup and that is not a "best practice", okay I will add this fixes in my next release. Hey maybe you can adding new features to Danland as well? You have done a great work for the community :)

Dan

Comment #4

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 1 February 2011 at 17:31

Status:

Patch (to be ported)

» Needs review

File	Size
danland-1047388-3.patch	5.35 KB

danland-1047388-3-whitespace.patch	17.5 KB

Hardcoding empty divs is never good, for both theming and semantic reasons. And doing it for the reason of compatibility between D6 and D7 isn't a good reason.

There were some weird whitespace issues in the last patch. I've attached two, one with just the required changes, the other with whitesepace fixes.

Comment #5

danpros CreditAttribution: danpros commented 1 February 2011 at 18:14

Status:

Needs review

» Patch (to be ported)

Hi Tim,

Wow we submitted a post on the same time, but I won a few second there. Congrats Dan!

Yes that is not a good reason :). I check the patches and the codes become very clean (not nasty). I will use all of this fixes for the next release. I will mark this as patch to be ported (for reminder). I will implemented it later, if somethings goes wrong I will post it in here.

Thanks,
Dan