Fix default page.tpl markup [#163723]

Drupal 6 gives us the ability to create .info files for our themes, and enables the creation of 'pure css' themes that include no markup or page.tpl files. That's awesome, but our default 'fallback' page.tpl file is now an old-school table based layout that's useless for the kinds of CSS driven templates that the feature promises.

This patch swaps in a slightly modified version of garland's page.tpl.php file and gives folks a good starting place for pure CSS work.

It can probably use some improvement -- I took out the page_body_class() function, as it was implemented in garland's template.php, but something like it is probably needed for clean multi-column support. In any case, ti would be absolutely tragic if we went to all the work of making this possible, but shipped with a default page.tpl file that made it useless.

Comment	File	Size	Author
#12	theme_7.patch	14.2 KB	eaton
#12
#9	theme_6.patch	14.23 KB	eaton
#9
#7	theme_5.patch	14.26 KB	eaton
#7
#4	pagetpl2.patch.patch	11.38 KB	Frando
#4
	simple_themes.patch	7.22 KB	eaton

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Frando CreditAttribution: Frando commented 1 August 2007 at 16:20

Status:

Needs review

» Needs work

This is definitely needed. And it's a great start.

Remarks from reading the new page.tpl.php line-by-line:

- In the patch, $base_path, $site_name, $logo, $site_slogan are sanitized in the template file. Ideally, that should happen in the preprocess function to make things easier for themers.
- I'd put the site slogan into a div
- A div with an id='navigation' wrapped around primary and secondary links would increase flexibility.
- I'd rename div id="center" to "main". It's not necessarily in the center of the page, isn't it.
- Remove the three divs that follow the center div ('squeeze', 'right-corner', 'left-corner'). They're really Garland-specific. We might want to leave one in ("main-wrapper") for increased flexibilty
- Many themes will need a wrapper div around sidebar-left, main and sidebar-right. So, we should better add it.
- Move footer out of the center/main div. It's quite garland-specific that the footer lives inside the main content div
- We do if($tabs) but if (isset($tabs2)) - why that? Seems to be confusing for themers.
- Remove the span class="clear" that is below the content. Wrap the content into a div id="content" class="clear-block" instead.

Comment #2

dvessel CreditAttribution: dvessel commented 1 August 2007 at 17:40

Wow, this almost slipped in. Would have been a shame. :)

Closely matching Garland is because it's the default theme? I dig Garland but it's pretty heavy on the markup. For whatever it's worth, I think it should contain as little as possible to have a basic layout possible. It should be more of a reference. If someone want's to mimic Garland, they can simply set it as their parent theme. Maybe not ideal.. dunno.

Comment #3

dvessel CreditAttribution: dvessel commented 1 August 2007 at 17:43

Oh, and forgot to mention that the theme registry for node is removed in that patch.

Comment #4

Frando CreditAttribution: Frando commented 2 August 2007 at 00:10

Assigned:	eaton	» Frando
Status:	Needs work	» Needs review

File	Size
pagetpl2.patch.patch	11.38 KB

So...
after some discussion on IRC, we all agreed that Garland's page.tpl.php is not really suited as a default template.
In the attached patch, the page.tpl.php is now based on a modified version of the page.tpl.php from the zen theme/zengine engine, which was specifically designed for that purpose (being a generic default template, that is).

A number of informational classes are added to the body tag. That allows themers to do context-specific theming even in CSS-only themes (e.g. a sanitzed version of arg(0) is added as a page-arg(0) class, so themers can e.g. theme admin pages differently from node pages just with CSS, without having to create anything but a style.css and a themename.info file).

The removal of the node theme info is on purpose, it was a critical bug that prevented CSS-only themes from working.

Comment #5

eaton CreditAttribution: eaton commented 2 August 2007 at 06:35

Status:

Needs review

» Reviewed & tested by the community

Wow.

This is actually really, really slick and much nicer than what I'd even hoped for. The extra classes make for some very cool potential tricks. Seeing as how this does NOT change any output for existing Drupal themes, only the output for themes with .info files but no page.tpl.php.

This fixes the underlying problem that was an emergency -- the fact that we were using a 'nasty for CSS table layout' as our fallback. It's RTBC, unless someone raises massive philosophical issues with it.

Comment #6

Dries CreditAttribution: Dries commented 2 August 2007 at 10:20

Status:

Reviewed & tested by the community

» Needs work

+            <h1 id='site-name'>

Can we use double quotes for these things?

Comment #7

eaton CreditAttribution: eaton commented 2 August 2007 at 15:07

File	Size
theme_5.patch	14.26 KB

1) Fix inconsistent double and single quotes
2) Make use of !empty() versus isset() versus if ($text) consistent.
3) Add ginormous block of PHPDoc at the beginning.

As per our IRC conversation I'm RTBCing this again as it is pure cleanup and documentation. No hurt feelings if there are objections to that. ;)

Comment #8

eaton CreditAttribution: eaton commented 2 August 2007 at 15:08

Status:

Needs work

» Reviewed & tested by the community

Comment #9

eaton CreditAttribution: eaton commented 2 August 2007 at 15:40

File	Size
theme_6.patch	14.23 KB

One more code style tweak in the comments. Thanks to dmitrig01 for pointing it out.

Comment #10

dvessel CreditAttribution: dvessel commented 2 August 2007 at 16:15

The way the sidebar classes are formed are a bit odd to me.

"two-sidebars sidebar-left sidbar-right" .. looking at that. How is it better than just providing one of the following: sidebar-left, sidebar-right, sidebar-both or sidebar-none.

A single class to determine the state is easier to select with css. Trying to manage multiple classes seems awkward. It may just be me, is there another use for this? IE doesn't support chained selectors ".two-sidebars.sidebar-left" but it doesn't look like it would be needed. Why the multiple classes?

And the "not-front" class doesn't seem necessary. Most would style for the front page if they had to, not the other way around.

Otherwise, awesome!

Comment #11

merlinofchaos CreditAttribution: merlinofchaos commented 2 August 2007 at 18:22

I don't see a problem with front and not-front, I think that's okay.

I agree that sidebar-none, sidebar-left, sidebar-right and sidebar-both are the way to go.

I think these body classes are all very useful and we should probably leave them. They will facilitate CSS-only themes. Facilitating CSS-only themes makes it so that designers who are good at CSS and don't want to work with templates don't have to. This is important.

This should be followed up by a very honest evaluation of some of the other very commonly seen HTML in our pages, to make sure that we can truly make a CSS only theme a reality.

The PHPDoc here is awesome.

Comment #12

eaton CreditAttribution: eaton commented 2 August 2007 at 18:49

Status:

Reviewed & tested by the community

» Needs review

File	Size
theme_7.patch	14.2 KB

After taking a careful look at the patch, dvessel's point is a good one. By including 'sidebar-left' and 'sidebar-right' when we have both enabled, it becomes useless as a standalone "what side is the single visible sidebar on" selector. Layouts that shift around based on which of ONE sidebar is visible would have to use the less-supported css chaining syntax. This patch changes one thing: ensures that 'sidebar-left' means 'only the left sidebar is visible'.

Comment #13

dvessel CreditAttribution: dvessel commented 2 August 2007 at 18:55

Status:

Needs review

» Reviewed & tested by the community

Another note: the body classes should be prefixed. It could easily interfere with other classes within the body.

Or, it should be documented that selecting the body class should include the body tag. "body.front {xx}" to minimize the unexpected.

Comment #14

dvessel CreditAttribution: dvessel commented 2 August 2007 at 18:57

Heh, or not. We can document it somewhere else. Don't want to hold this up.

Very nice!

Comment #15

eaton CreditAttribution: eaton commented 2 August 2007 at 19:06

Also, for those interested in the details of why "sidebar-left" versus "sidebar-both sidebar-left" is a tricky issue, http://www.ryanbrill.com/archives/multiple-classes-in-ie/ has an excellent description of the IE CSS selector bugs that make the problem complex. The best way for now is to avoid classes the must be combined on a single element to be meaningful, and stick to single free-standing classes.

Comment #16

dvessel CreditAttribution: dvessel commented 2 August 2007 at 20:00

Thanks Eaton for explaining. I wasn't so clear.

This should be followed up by a very honest evaluation of some of the other very commonly seen HTML in our pages, to make sure that we can truly make a CSS only theme a reality.

Definitely, while converting templates I didn't think about this. All I did was match the original but maybe I can slip it in.

It would be great if there was a central way of defining classes. Let it clean up and push it as another variable into any theme function where appropriate. Maybe D7?

Comment #17

Frando CreditAttribution: Frando commented 2 August 2007 at 20:08

The changes in the last patches and especially the PHPDOC look great. We discussed this in IRC once again and everyone seemed to agree with the body classes now, especially with the sidebar classes. RTBC +1 from me.