Posted by danpros on February 3, 2010 at 10:36pm
| Project: | Drupal.org CVS applications |
| Component: | Miscellaneous |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | theme review |
Issue Summary
This is the second time I tried to get a CVS account, at the first opportunity I did not include a link to a screenshot or demo site of the theme, so that does not meet the mandatory requirements. I am not a Drupal, CSS or PHP master, but I am ready to contribute to Drupal in any way I can, and currently I've made a theme for Drupal, and wanted to share it to the Drupal users.My Drupal theme name is Simpler theme, it's minimalist, clean, tableless, multi column and fluid width theme. Designed for cross-browser compatibility; work perfect in Firefox, IE 6, IE 7, IE 8, Safari, Opera and Google Chrome.
This theme is CSS 2.1 and XHTML 1.0 validated, with 5 regions (left sidebar, right sidebar, content top, content bottom and footer), supports custom logo and favicon, primary links and secondary links.
It's also support user picture in profile, comments and post; and support other features like site name, slogan, mission and custom search box on the right top.
Here's the demo site from this Simpler theme: http://www.xpsdev.com
Comments
#1
Here's the code of the theme.
#2
Hello, and thank you for applying for a CVS account.
I am adding the review tags, and soon somebody will review your theme.
#3
Thanks again. Here's the small screenshot of the theme, just in case if the demo site can't be accessed due to server problems.
#4
I installed your theme and tried it out. It would be nice to have header region. I wouldn't describe it as "tableless" ... here is just a snip of your page.tpl.php:
<table border="0" cellpadding="0" cellspacing="0" id="content"><tr>
<?php if ($left) { ?><td id="sidebar-left">
<?php print $left ?>
#5
Sorry for wrong description, I'm still learning English and sometimes still make mistakes like that :)
Here's the theme with header regions.
#6
Well first off, I would get rid of tables if at all possible. Though it is not against Drupals standards for coding (to my knowledge anyway), its just a bad practice in general. Using tables to lay out the content of a site is a thing of the past. I like think the design, simple, yet functional.
The main thing you need to do in order to follow Drupals coding standards is make sure that all your blocks of php end with a simicolon. Example, here is an excerpt from line 24 of your page.tpl:
<?php print $front_page ?>It looks like about 30% of the PHP in your .tpl files is formatted this way, this should be fixed. In order to follow Drupals coding standards it would have to be done as follows:
<?php print $front_page; ?>#7
Hmm.. I keep the table, lots tableless theme out there so its a choice for the user if they want a table layout theme.
I fix the code to follow Drupal's coding standards.
#8
You have an empty PHP declaration on line 11, that should be removed.
You should remove lines 22, 24 of your .info file, the version and project attributes are added by drupal.org when its uploaded through CVS. More information here - http://drupal.org/node/171205
When uploading a theme to the Drupal.org repository, it is important to make code as clean and readable as possible. Even above and beyond the documented Drupal coding standards. The PHP syntax you are using, though completly valid, is not what is recommended. Here is an excerpt of on of your variables
<?php if (isset($primary_links)) { ?><?php print theme('links', $primary_links, array('class' => 'links', 'id' => 'navlist')); ?><?php } ?>Though that is completely functional and actually a little shorter, but remember, this theme is available for the world to download, you wanna make the code is as clear as possible. @kiamlaluno writes about this some here. In Drupal themes it is encouraged to write your
ifstatements like this :<?php if (isset($primary_links)) : ?><?php print theme('links', $primary_links, array('class' => 'links', 'id' => 'navlist')); ?>
<?php endif; ?>
Most of your logic is written like the first example, I would fix this.
#9
Hello tlattimore, thanks for your advice, it really helps.
I've done as you suggest.
#10
You need to remove information from the CVS headers of your files, this information is added by Drupal.org when uploaded through CVS.
This:
<?php// $Id: page.tpl.php,v 1.28.2.1 2009/04/30 00:13:31 goba Exp $
?>
Should be changed to this:
<?php// $Id
?>
Read about it here - http://drupal.org/coding-standards#cvs
#11
Thanks for your review, I already remove that.
Here's the zip file.
Regards,
#12
Well, still not a fan of the table based layout. From my are understanding they are bad for SEO and accessibility, but to each his own I guess. I don't see anything else that really breaks Drupals coding standards.
#13
Hello, is a certain satisfaction if we can overcome the weakness of a thing, I just tried to do the same. Next time I'll make a theme without tables, and who knows it could be the next Zen :)
Thanks again tlattimore, you gave me a lot of knowledge about the Drupal coding standards.
#14
#15
Automatically closed -- issue fixed for 2 weeks with no activity.