Polish up Pushbutton theme

IMO, Pushbutton has been ugly for some time now. Its appearance, while bad, does not today rise to a critical.

While I applaud your sentiment, and agree that it should be moved out of core, if history (or poll module) has taught us anything, it is that this is not the time to be making big changes in Drupal 6 by removing core themes (or modules).

Pancho: does it become ugly in Drupal 6? I don't think so... Feel free to push the button for its removal (pun intended) in Drupal 7, I could not agree more. But for Drupal 6, it is way too late. I would say all themes except Garland/Minnelli are sub-par (now that we have Garland/Minelli, the comparison just blows them away). Unfortunately the core theme SoC tasks did not result in actual core themes.

Actually I much prefer it to Bluemarine...

Oh yes I see what you mean about the degradation in D6. Go for it!

Warning: this is my first patch review, so I'm sorry if I've done this improperly in some fashion. However, this is something I can do and want to do, so here goes.

I tested this against HEAD, with Pushbutton as the main theme. First of all, I got this message:

patching file themes/pushbutton/block.tpl.php
patching file themes/pushbutton/page.tpl.php
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file themes/pushbutton/page.tpl.php.rej
patching file themes/pushbutton/box.tpl.php
patching file themes/pushbutton/node.tpl.php
patching file themes/pushbutton/style.css
patching file themes/pushbutton/style-rtl.css

As far as the actual changes, there are two images attached: a before and after (the before picture was taken after I reversed the patch...). The actual layout looks better, but the top banner part looks more messed up, IMO.

Nice work, Pancho, I like Pushbutton a lot, although it may be better for contrib in D7.

Catch brought it to my attention something about moving the image files. I'll test in a moment, but resetting for the moment. Sorry about that!

Okay, here goes. Here's an official review, with the images moved. I'm sorry about that Pancho, I should have read the issue a bit more carefully.

Well, first of all, it's obnoxious to select all those files and move them, although I presume if we patched this that would be fixed?

Also, that error still stands true against HEAD:
$ patch -p0 < pushbutton_1.patch
patching file themes/pushbutton/block.tpl.php
patching file themes/pushbutton/page.tpl.php
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file themes/pushbutton/page.tpl.php.rej
patching file themes/pushbutton/box.tpl.php
patching file themes/pushbutton/node.tpl.php
patching file themes/pushbutton/style.css
patching file themes/pushbutton/style-rtl.css

But the changes look good, although I think there is a bit more to do. Is it just me, or is a core theme not working like this critical?

Alright, I am bumping this to critical. If this is a newbie mistake, my apology, just trying to help here. That said, although Pushbutton is rarely used (and perhaps it should go contrib for D7, as suggested earlier in this thread and I am sure elsewhere), the weirdness in this theme is just not up to the Drupal standard and needs to be fixed. Pancho's patch is a good start, although it needs to be updated for HEAD, I think.

See http://drupal.org/files/issues/beforepatch.png for what it currently looks like in head.

IMHO, if it goes to contrib status (I vote against that, so that means it will happen), it should have an owner at least for a little while, otherwise it is just being thrown away.

nancyw, I don't know enough to make an educated decision about what should be done, so you may very well be right.

Pancho, glad to know my review got you going.

Thanks, Pancho. I actually have one site where I use Pushbutton.

Minor spacing issues, distorted footer and the odd header graphic spacing doesn't make it critical IMO.

OK, I have a bit of responsibility here as that's my finger on the button. I'm tied up next week, but my colleague Chris will do some work on it on Tuesday, and I'll pitch in at the weekend.

crosspost

I still think this is 100% critical; although the problems might be minor, I do not think D6 should be shipped like this, especially when a patch is in progress.

Sorry dvessel, I overlapped you and didn't mean to undo your changes. I am not an expert, so I'll leave it up to others to decide.

Let's get this rerolled for D7, eh?

Now that all the table-based themes have been moved out of core, can we mark this as won't fix?

Since it has patches, suggestions and all, why not move it to the contrib project?

The latest code in CVS has been updated to work with the latest Drupal 7. Patches welcome. (See also: #881442: Find new maintainer)