Simple Clean

Hello, and thank you for applying for a CVS account.

As per requirements, the motivation needs to include the description of the theme features, a screenshot, and when possible a link to a demo site.

Hi,

This looks awesome. I'm volunteering here a bit and plan to post again tomorrow after I look at this some more. Before I do I was wondering where $splash comes from (in the page template) - I am not familiar with this.

Sincerely,

Matt Winters

Oh sheesh. Sorry. I should have finished looking through everything first. I was busy wondering if there was some fancy new variable or something. :)

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

The following code in page.tpl.php is potentially vulnerable to XSS as it outputs direct link titles, rather than using the l() function which properly escapes link titles. This needs to be fixed prior to approving this Git application.

      $show_submenu = theme_get_setting('simpleclean_show_submenu');
      if ($show_submenu) {
        $tree = menu_tree_page_data('primary-links'); 
          foreach ($tree as $key => $mi) {
            if ($mi['link']['in_active_trail'] && $tree[$key]['below']) {    
              $menu = menu_tree_output($tree[$key]['below']);
              $menu = $menu . '</div>';
              $link = $mi['link']['link_path'];
              $title = $mi['link']['title'];
              print "<div class=\"submenu\"><h2 class=\"title-subnav\"><a href=\"/$link\">$title</a></h2>";
            }
          }
      print $menu;
      }

I'd also encourage you to avoid using logic in your templates like the above or the following code and put it into a template_preprocess_node() instead:

      <?php $account = user_load(array('uid' => $node->uid)); if (!empty($account->signature)) { ?>
        <p><?php print check_plain($account->signature); ?></p>
      <?php } ?>

Trying to standardize tags in the review queue.

Beginning review of this theme as part of the Drupal PDX User Group code review sprint.

Changing component to "theme."

Hi acke!

This looks really good and, from what I can tell, meets the requirements for becoming a full project. I was going to test this multiple browsers today, but when I checked http://simpleaclean.happiness.se/ it looked as though your demo site was no longer online. Otherwise, I only saw minor issues with it, and definitely not big enough problems to prevent it from being promoted to full project at this point.

Here are a few observations.

Strong points:

• Minimalism is a great design technique, thank you so much for contributing this type of design to the Drupal community! It is refreshing.
• Lots of documentation at top of template files - this is EXTEREMELY helpful - thanks!
• I like how you use clean font faces not only in the headers and body copy, but even in the text fields. The font face and rounded corners of the User Login fields are particularly fun. Have you thought about applying those settings more generically with classes rather than IDs, so that this effect applies to more parts of the site? Right now your CSS selector makes it very specifically styled for the login form:
  #sidebar #edit-pass, #sidebar #edit-name
• I like how you used -moz-border-radius, -webkit-border-radius, and border-radius for maximum cross-browser compatibility, and to avoid unnecessary loading of images to create the rounded-corner effect.

Minor syntax issues:

• Inconsistency in format of if statements. Some use curly braces:
  if($var) {
}
  Some use another syntax:
  if($var):
endif;
  Might be easier to read if it were consistent. Also, some extra spaces in a few places, e.g. if ($logo) : - extra space between ($logo) and :.
• Please remember to end lines of code with a semi-colon where appropriate, even in template files. For example:
  print $header;
  See Drupal coding standards for details.
• Coder module suggests removing whitespace at the end of each line of code.
• Secondary lines of comments should be indented one space. For example, in theme-settings.php, replace:
  

  /**
  * @file
  * Implementation of THEMEHOOK_settings() function.
  *
  * @param $saved_settings
  *   array An array of saved settings for this theme.
  * @return
  *   array A form array.
  */
  

  with:

  /**
   * @file
   * Implementation of THEMEHOOK_settings() function.
   *
   * @param $saved_settings
   *   array An array of saved settings for this theme.
   * @return
   *   array A form array.
   */
  

CSS notes:

• For some reason, it appears you have set the background color of the search field itself (when it appears in the sidebar) to the same background color as the block, so that the field itself is not visible:

  #sidebar .block-search .form-text {
    background-color: #EEEEEE;
  }

  Would it make sense to make this more visible by setting it to white? Also, would it make sense to change the selector so that, if people choose to put this block into a different region of their pages, the styles still apply?

• In general it looks as though most of the CSS selectors use IDs rather than classes, so any new blocks or other markup for which you have not specifically accounted will not be styled. Might make sense to switch to classes to provide greater flexibility in new content or when blocks are assigned to different regions.

Those are all of the major issues I noticed. Unless someone else in the community sees any issues, could someone with project promotion rights promote this to full project?

Wow! Thank you Mattias for your work, thank you Dave Reid for your review, and thank you danmuzyka for your super review!

Mattias, you've been fixing things right along so i'm confident you will make danmuzyka's suggestions.

Please also note that all control structure statements, including if statements, should have a space both before and after their parenthetical condition:

if ($var) {

Congratulations! You can now promote sandbox projects to full status ones. When you would like further review for any theme or module please do ask at http://groups.drupal.org/peer-review/requests

Further cleanup issues introduced here and new ones can be filed on your project issue queue. Thank you for your contribution and i look forward to seeing its refinement and development and your continued involvement in Drupal!

_{benjamin, agaric}