(See http://code.google.com/p/google-highly-open-participation-drupal/issues/...)

Original issue text:

This task is to convert Drupal’s core “bluemarine” theme to a tableless layout. This theme unfortunately still uses tables for the page layout.

Your task is to:
- Convert bluemarine to a tableless, CSS-based layout.
- Find flaws in the current design (malpositioned elements, too small margins, overflowing text, ...) and fix them.

When converting to a tableless layout, please keep in mind that bluemarine is often used as a “base theme” for creating more sophisticated designs. You will have to put particular focus on creating HTML markup and CSS code that is easy to understand and doesn’t rely on special hacks or complex neat tricks. CSS newbies shouldn’t have a hard time modifying or hacking bluemarine in order to make it suit their needs.

The final deliverable is a cross-browser compatible (IE6+, Mozilla, WebKit, Opera), CSS-only version of bluemarine in the form of a patch file. The task is considered to be sucessfully completed once this patch is marked “ready to be committed”. The patch should be created against the most recent HEAD version in CVS, not a downloadable package.

CommentFileSizeAuthor
#36 bluemarine-right-sidebar.zip19.44 KBspangaroo
#32 bluemarine_to_css_4.patch8.36 KBboombatower
#29 bluemarine_to_css_3.patch8.81 KBboombatower
#23 bluemarine_to_css_2.patch8.77 KBdvessel
#22 patch.txt7.34 KBboombatower
#19 bluemarine_to_css.patch8.13 KBdvessel
#16 patch.txt13 KBboombatower
#14 patch.txt13.23 KBboombatower
#11 patch.txt11.72 KBboombatower
#9 patch.txt11.72 KBboombatower
#7 patch.txt12.01 KBboombatower
#5 page.tpl_.php_.txt2.51 KBboombatower
#5 style.css_.txt6.25 KBboombatower
#3 page.tpl_.php_.txt2.44 KBboombatower
#3 style.css_.txt6.1 KBboombatower
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JirkaRybka’s picture

JirkaRybka CreditAttribution: JirkaRybka commented

OK, all fine, but I would like to suggest that we keep at least *one* tabled theme in core, to give the minimum of a choice to people, who still don't want their site's layout broken on every smaller screen/browser window, or older browser. I'm not going to elaborate here (no intention to bring up that "holly war" over again), but since I'm running 1024x768 screen, I haven't seen a single reliable tableless theme yet. Garland with 2 sidebars used breaks on core administrative pages even, so that's an example.

Let me say it again - no intention to hijack this thread. Just a small reminder, that keeping a choice between tabled/tableless might be a good idea.

add1sun’s picture

add1sun CreditAttribution: add1sun commented
Status: Active » Postponed (maintainer needs more info)

Claimed by j.boombatower

boombatower’s picture

boombatower CreditAttribution: boombatower commented
FileSize
style.css_.txt6.1 KB
page.tpl_.php_.txt2.44 KB

I have converted the theme, from the HEAD, to tableless CSS. I had a few problems with IE6 related to the box model, but I believe I have everything working.

I left the previous names, for example: header, footer in place in hopes that any subclasses would be unchanged.

I tested the theme on:

  • Firefox 2.x
    • SUSE 10.2
    • Windows XP
  • IE6 on Windows XP

I do not have access to test the theme in WebKit, Opera without installing them, so if someone could test the theme on those two that would be great.

boombatower’s picture

boombatower CreditAttribution: boombatower commented

I noticed some problems with the menu area when the search is enabled or sub-menus are being used. I will work on fixing those issues.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
FileSize
style.css_.txt6.25 KB
page.tpl_.php_.txt2.51 KB

Well, dinner gave me a few ideas which I think have fixed the problem.

aclight’s picture

aclight CreditAttribution: aclight commented
Status: Postponed (maintainer needs more info) » Needs work

The task description requires that your work be submitted as a patch to Drupal HEAD. There is a handbook with instructions for how to do this at http://drupal.org/patch/create

If you have problems making a patch, feel free to drop in to the #drupal-ghop or #drupal IRC channels on irc.freenode.net. There are usually people there willing to help you create a patch if you need it.

Also, make sure that your code follows the Drupal coding standards at http://drupal.org/coding-standards
Specifically, watch out for tabs (we use two spaces).

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs work » Postponed (maintainer needs more info)
FileSize
patch.txt12.01 KB

I believe I fixed the coding standard issues. This is the first time I have had to generate a patch so I am not sure if this is correct.

EDIT: Looks like I accidentally changed status.

aclight’s picture

aclight CreditAttribution: aclight commented
Status: Postponed (maintainer needs more info) » Needs work

When your patch is ready for review make sure to mark it as such (patch (code needs review)) and not as active (needs more info). That status is usually used for bug reports where someone needs more information to reproduce a problem.

Your patch also still has some tabs, so you need to fix those.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs work » Needs review
FileSize
patch.txt11.72 KB

Not sure how I missed those tabs. Sorry about that.

I hope this is what you are looking for.

aclight’s picture

aclight CreditAttribution: aclight commented

I'll try to review this tomorrow. But I just wanted to check to make sure that you've checked the HTML and the CSS that comes out of this to make sure that both validate correctly.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
FileSize
patch.txt11.72 KB

Yes, I checked them both periodically as I worked. There appears to be a spelling error in the CSS that I have fixed with this patch.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs review » Needs work

A few notes about the direction of the change:

1.) Any classes or ID's should not be using underscores. Use dashes instead. logo_small/logo_large & columns_1/2/3

2.) I don't see a need for the different logo classes. Using it to add extra spacing when the search box is present is really needed? This was never originally checked for.

3.) The extra div wrapper within the menu container isn't needed.

4.) The #body "column_1/2/3" classes can be replaced with $body_classes by placing it within the body tag. It'll output a bunch of classes including the state of the left & right columns. This way it can be more flexible by being able to use descendant selectors for the side bars themselves instead of being limited to the center column and there would be less "logic" in the template.

5.) There a couple of instances of "vertical-align:top". It only applies to tables.

6.) Use of "body" ID deep inside the layout just seems odd. There's the page <body> and that can be confused easily. How about just "main-content" and removing the "main" ID wrapper.

7.) How about using holy grail layout or anything else proven to work reliably. The conversion currently breaks in IE7 with the right column clearing past the main content. Looks fine in IE6. Would be nice to keep the widths of the side bars fixed. Resizing the window can really make things ugly when using percentages. Main content area should be fluid though of course.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
  1. Will fix
  2. There is no way that I know of to vertically center the logo text without doing something like that. Divs can't inherit their parents height so it is hard to vertically center what is inside. The logo also doesn't cause the div to expand, only the link, so the height needs to be set and the height needs to change when the search box is added unless you wish to have it always that size. If you have any ideas on how to make this work that would be appreciated.
  3. That was a left over from some experimenting, will remove
  4. Will fix
  5. Must have missed those when I was trying to clean up the old CSS - will fix
  6. Will change
  7. I will play around with that. I'm not sure exactly how you want that implemented. The layout appears to have the same problems I have seen with all others: you can't make the three columns expand to the size of the largest. Would you rather I cut of the content on the side panels when necessary (example 3) or just let them all float and the background colors will not extend all the way (example 4).
boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs work » Needs review
FileSize
patch.txt13.23 KB

I changed everything you mentioned and believe this is what you asked for.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs review » Needs work

Boombatower, this looks great. One little problem though. There must be an extra wrapper for the columns so IE6 calculates the positioning properly on load. With your latest patch, the left column pushes into the main content area. Resizing the window snaps it back into place. Just another odd quirk.

Somehow, providing the wrapper helps IE6 calculate it correctly.

Your current patch: (bad for ie6)

<body>
  ...
  <div id="main-content" class="column">
   ...
  </div>

  <div id="sidebar-left" class="column">
    ...
  </div>

  <div id="sidebar-right" class="column">
    ...
  </div>
  ...
</body>

This works:

<body>
  ...
  <div class="column-layout"> <!-- ie6 needs this wrapper -->
    ...
    <div id="main-content" class="column">
     ...
    </div>

    <div id="sidebar-left" class="column">
      ...
    </div>

    <div id="sidebar-right" class="column">
      ...
    </div>
    ...
  </div>
  ...
</body>

Then add the padding to .column-layout instead of <body>. This will definitely fix it. I'd say it will be RTBC after that.

The only other minor problem is that the columns don't extend down the length of the page but I think that can be done in a follow up patch by someone else. It does make it more hackish though so I'm not sure what others will think about that.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs work » Needs review
FileSize
patch.txt13 KB

Interesting that the holy grail layout layout doesn't work on IE6. My previous layout worked on 6, but not 7. Oh, the wonders of IE, always making life easier.

I made the changes that you requested. As for the the columns not extending I mentioned that in point 7 of #13. That is the main problem I have found with tableless layouts. The only fixes I have seen for that require JavaScript (yet another hack).

Note: By adding that wrapper it removes three of the layout css declarations, making it much easier to modify.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

Forgot to mention that it will also need a IE7 hack with the change I mentioned above.

/* IE7 Fix for HasLayout */
html>body .layout-columns {
  zoom: 1;
}

It cannot be seen by IE6 or it'll break that. It's looking hackish but it's only two selectors, one for each version of IE. Not sure it warrants extra style sheets for conditional includes.

[edit: I'll add this myself as I'm testing. Thanks.]

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs review » Needs work

I'm not exactly sure what I'm supposed to do, that is why I haven't responded. Do we just need to decide on some conditional CSS or what?

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs work » Needs review
FileSize
bluemarine_to_css.patch8.13 KB

Sorry for the delay boombatower. There were serious issues with LTR layout in IE6/7 with the holy grail technique.

I'm not sure I'm allowed to do this (since it's assigned to you) but here's a patch. Tested with IE7, IE6 and Safari in both language directions. If another can verify that it's solid with FF and Opera, I think it'll be ready for commit.

Boombatower, if you see any other issues with this I'll leave it up to you. I just didn't want to go in cycles endlessly with this type of issue. :: curses IE ::

btw, the IE hacks have been removed. No longer necessary.

boombatower’s picture

boombatower CreditAttribution: boombatower commented

Works in Firefox, but it seems you increased the header size. If that is alright it seems to work on my end.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

How about fixing that. And I'll mark it ready. ;)

boombatower’s picture

boombatower CreditAttribution: boombatower commented
FileSize
patch.txt7.34 KB

I just changed it slightly.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
FileSize
bluemarine_to_css_2.patch8.77 KB

There was another issue with the RTL layout for both IE versions where the logo, name and slogan would get cut off. The only fix I could find was to do a little dirty trick of setting a width for the container of those elements and using negative margins. Not fool proof though. ..sigh

#branding {
  float: left; /* LTR */
  margin: 0 -100% 0 0; /* LTR */
  width: 100%;
}

It's either that or the header positioning must change. Without knowing the exact width of the slogan, name and navigation links makes it extremely tricky.

Here are a few screen shots for four browsers in both LTR and RTL layouts screen shots.

boombatower’s picture

boombatower CreditAttribution: boombatower commented

So where do we stand and is there anything else I need to do?

dvessel’s picture

dvessel CreditAttribution: dvessel commented

boombatower: fix any other issues you find. We need a third person here to verify to mark it RTBC.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

Could we get someone to review this please? boombatower needs to move on.

aclight’s picture

aclight CreditAttribution: aclight commented
Status: Needs review » Needs work

The only issue I see is that the description of the theme still says:

Table-based multi-column theme with a marine and ash color scheme.

So since this is no longer table based, that needs to be changed. I think that's done in the .info file of the theme.

After that's fixed I would say RTBC and the student gets credit.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

Sounds good. Patch boombatower?

boombatower’s picture

boombatower CreditAttribution: boombatower commented
FileSize
bluemarine_to_css_3.patch8.81 KB

I changed the .info file to reflect the fact that the theme is no longer using tables.

This patch includes that change along with the other changes required to convert the theme to a tableless format.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Reviewed & tested by the community

Ok. This last patch addresses aclight's concern. Marking RTBC here, and closed on the Google tracker. Thanks!!

Could you please upload your final patch to http://code.google.com/p/google-highly-open-participation-drupal/issues/... as well?

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work

Unfortunately, this patch no longer applies to CVS HEAD. Please re-roll. Thanks.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Assigned: Unassigned » boombatower
Status: Needs work » Reviewed & tested by the community
FileSize
bluemarine_to_css_4.patch8.36 KB

Re-rolled. The page template fits on a page now ;)

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

I've added an entry to the CHANGELOG.txt and committed this to CVS HEAD. Job wel done.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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

ravisagar’s picture

ravisagar CreditAttribution: ravisagar commented
Issue tags: +GHOP

When I applied the patch using patch -p0 < bluemarine_to_css_4.patch I got the following error. Am I doing anything wrong.

patching file themes/bluemarine/page.tpl.php
Hunk #1 FAILED at 11.
1 out of 1 hunk FAILED -- saving rejects to file themes/bluemarine/p age.tpl.php.rej
patching file themes/bluemarine/style.css
Hunk #2 FAILED at 70.
Hunk #3 FAILED at 198.
2 out of 4 hunks FAILED -- saving rejects to file themes/bluemarine/ style.css.rej
can't find file to patch at input line 272
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: themes/bluemarine/style-rtl.css
|===================================================================
|RCS file: /cvs/drupal/drupal/themes/bluemarine/style-rtl.css,v
|retrieving revision 1.5
|diff -u -r1.5 style-rtl.css
|--- themes/bluemarine/style-rtl.css 17 Dec 2007 15:05:09 -0000 1 .5
|+++ themes/bluemarine/style-rtl.css 6 May 2008 22:53:51 -0000
--------------------------
File to patch:

spangaroo’s picture

spangaroo CreditAttribution: spangaroo commented
FileSize
bluemarine-right-sidebar.zip19.44 KB

Thank you to j.boombatower for designing a great tableless bluemarine theme. He also took the time to forward the complete theme which I thought was very kind.

I modified the template because I needed the sidebar floated to the right. It seems to work okay in IE7, FF 3.0.9, and Safari 3.2.1.

Modding the CSS is still new to me so if you find any layout problems please let me know because I'm interested in improving if I can.