ICE BLUE

It would be nice to have the git address in the description so reviewers would not have to go to your sandbox page to get it.

Review of the 6.x-2.x branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Remove all old CVS $Id tags, they are not needed anymore.
  

  js/jquery.pngFix.js:1:// $Id: jquery.pngFix.js,v 1.1 2010/07/19 22:25:16 danprobo Exp $
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./css/ie7.css ./css/style.css ./css/jquery.selectBox.css ./js/cufon-yui.js ./js/jquery.selectBox.js ./js/jquery.pngFix.js
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Is there a reason for naming the branch '6.x-2.x ' ?

Review of the 6.x-2.x branch:

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  
  README.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

While there is, in theory, no reason why your branch cannot be 2.x, it's pretty confusing if 1.x is never released.

However, the actual work MUST be in 6.x-1.x or 6.x-2.x, not 6.x-2.1.
When your module is released, Drupal.org packages the 6.x-2.x as dev-release and when you are actually ready
to release a stable version, you will create a tag 6.x-2.0 for example.

If you want to have some experimental code there, you can also create additional branches and name them how you like.

Please read this and this.

You still have code in the master branch. Master branch is deprecated and there should not be any code. Read this.

More importantly, it seems to me that you have included non-GPL libraries(in the js directory). Those are not allowed, you need to either put instructions how to download them or you can use Libraries API to handle that stuff.

In theory, GPL and dual licensed stuff can stay there. However, a good practice is to keep ALL third party libraries out of the repository.
You can read more on licensing here.

Ok, i think this is codewise RTBC, however, since i'm no theming expert, i'll paste some things that i'm not quite sure are supposed to be implemted/overridden like this, so the final reviewer(or someone else) can comment:

Template.php:

if (!$user->uid) {                                                          
    $output .= '<a href="user/register" class="hd-link"><img src="/sites/all/themes/iceblue/images/open-an-account.jpg" alt="open an account" /></a>';                           
  }                                                                           
  else {                                                                      
    $output .= t('<p class="user-info">Hello !user</p>', array('!user' => theme('username', $user)));
 
      }

and

/**
 * Override or insert variables into all templates.
 *
 * @param $vars
 *   An array of variables to pass to the theme template.
 * @param $hook
 *   The name of the template being rendered (name of the .tpl.php file.)
 */
/* -- Delete this line if you want to use this function
function iceblue_preprocess(&$vars, $hook) {
  $vars['sample_variable'] = t('Lorem ipsum.');
}
// */

BUT, before i mark it as RTBC, clean up the repository, you now have 1.x, 1.0, 2.x.. and the 2.x still has the illegal js there.

I recommend you remove everything except the 1.x(and the README.txt from MASTER) completely.

looks good.

Ok, marking as RTBC but before anyone changes to "fixed", please check #12 and comment if necessary.
Thanks for the contribution. While waiting, please help by reviewing other modules in the queue.

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• theme-settings.php: "Theme implementation to display a single Drupal page.": that comment seems wrong here.
• theme-settings.php: the include_once should only be done if you need the code, i.e. in the function.
• template.php: @file comment seems to be copied from Zen, please replaced it with your own comments.
• ie7.css: indentation errors, please see the CSS coding standards http://drupal.org/node/302199 . Please also check the other CSS files. style-f.css for example.
• Git tags: you have a 6.x-1.x branch but 6.x-2.0 tags? Just remove those tags as long as your project is a sandbox.
• Make sure to take care of more useful commit messages, see http://drupal.org/node/52287

Marking as RTBC.
While waiting, please help by reviewing other modules in the queue.

blockers

windows file permissions

There are executable files in the repository. See Changing file permission masks on Windows.

direct inclusion of javascript in template files

this is a major no no. you should not have to edit any of the templates for what you have done with the site. remove all the added javascript and add it to where they are meant to be.

hard coded slider images

??

hard coded credits, hard coded this hard coded that

no. It seems some of this stuff should be offloaded to a module that goes along with the theme, or at the very least, a whole bunch of theme settings. (provide link to google+, ok setting exists, display in the theme). please review the coding standards on commercial credit in themes and modules.

hard coded javascript paths

these must not be directly included in the tpl.php files anyway, but the path should also not be required to be in your theme directory. instead they should rely on the libraries module.

template.php

links

links must be generated with the l() function so that the setting of 'clean urls' is not hard coded into the site.

html in translated strings

do not include html in the t() strings - standard exceptions from the coding standards apply.

'<p class="user-info">' . t('Hello !user', array('!user' => theme('username', $user))) . '</p>'

hard coding again

the image path is hard coded - use the theme path variable that has been passed to this function to construct the image file path

highly recommended

a zen sub-theme

this fact should be made much more obvious that the user also needs to download and install the Zen theme. this may seem obvious to you, but it isn't known or obvious to a lot of people installing a theme. you have not included it in the 5 step install instructions. the install instructions also insist that everything go into sites/all/themes/iceblue which is incorrect. the libraries module should be used to allow these to be placed anywhere the user wishes, and the paths should not be calculated in the tpl.php files.

At this point I'm going to recommend that this project enter the category of Single Project Promote as I would suggest that you need additional pointers to ensure that future projects are on the right track and don't include such errors.

I should also mention that there are quite a number of files in the images directory that are not referred to at all and should be removed.

I also tried to find where the columns were setup (normally in the the layout.css or layout-fixed.css file), but this seems to have been removed. The column setup is actually one of the most important parts of the Zen themes and as best as I can tell, it has been completely destroyed. None of the negative margins are in place, and the page content is no longer optimal for SEO. In fact, it seems almost all of the zen of Zen theme has been removed.

Sorry you can only have one application in queue (http://drupal.org/node/1404332).
Please close at least one of them.