[D7] Min Theme

Timeout when invoking pareview.sh for http://git.drupal.org/sandbox/c-whitman/2157739.git at http://pareview.sh/pareview/httpgitdrupalorgsandboxc-whitman2157739git

Do you have any third-party files committed? 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Currently, Drupal 7 is using jquery v1.4.4. Your theme is loading 2 version of jquery 1.10.2(your theme) and 1.4.4(Drupal built in).

html.tpl.php Line 37:
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>

This will cause a lot of issues for other Drupal modules that using jquery. Please find another way to alter jquery loading in your theme.

@chadwhitman,

1) In your .info file:

- The 'header' region is commented out.. Is that intentional?
- You have added the screenshot key. Use this key only if your thumbnail file is not called "screenshot.png" or if you want to place it in a directory outside of your theme's base directory (e.g. screenshot = images/screenshot.png)

2) In your main.css:

- You have used @import to include the 2 font families:

@import url("http://fonts.googleapis.com/css?family=Lato");
@import url("http://fonts.googleapis.com/css?family=Crimson+Text");

[Don't use @import to import css files. One reason being: Using @import within a stylesheet adds one more roundtrip to the overall download time of the page.]

Here is a preferred way to add an external style sheet to your Drupal 7 theme:

Add them in below format to your theme’s template.php file:

function yourtheme_preprocess_html(&$variables) {
drupal_add_css(
'https://fonts.googleapis.com/css?family=Droid+Serif:regular',
array('type' => 'external')
);
}

You have to fix line 9 in template.php file.

FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
9 | ERROR | Missing function doc comment

That alone is surely not an application blocker, any other issues you found in the code or is this RTBC otherwise?

Not sure but try to put a comment for min_preprocess_html function like below:

/**
 * some comment here...
 */
function min_preprocess_html(&$variables) {
  drupal_add_css(
  'http://fonts.googleapis.com/css?family=Lato',
  array('type' => 'external', 'description' => 'add Lato')
  );
  drupal_add_css(
  'http://fonts.googleapis.com/css?family=Crimson+Text',
  array('type' => 'external', 'description' => 'add Crimson')
  );
}

Git Clone Command
Your git clone command is for you, and will not work for others.
Project page
No. Please take a moment to make your project page follow tips for a great project page.
README.txt
No: Please take a moment to make your README.txt follow the guidelines for in-project documentation and the README.txt Template.

Error in http://pareview.sh/pareview/httpgitdrupalorgsandboxc-whitman2157739git please verify

3rd party code/content

No:

The theme/module comes bundled with normalize.css and boilerplate.css.
These appears to be third party code/content. Third party code/content/assets is not generally allowed on Drupal.org and should be deleted.
These particular assets are made available under the MIT license. This is considered a GPL-compatible license, which is usually allowed, but explicit permission to use from Drupal.org webmasters is still required.
This policy is described in the 3rd party libraries and content on Drupal.org. It also appears in the Drupal Git Repository Usage policy you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.
The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Licensing

No. Please remove the licensing statements from REAME.md. Drupal will add the appropriate version automatically during packaging so your repository should not include it. See also: licensing requirements

Review of the 7.x-1.x branch

Functional testing:

• No problems

PAReview:

• Template.php:
• Line 5: There must be exactly one blank line after the file comment.

Coder review (using the minor (most) option to see everything.

• script.js
• File: @file block missing (see http://drupal.org/node/1354#files)
• template.php
• File: @file block missing (see http://drupal.org/node/1354#files)
• Line 6: Docblock should be immediately above min_preprocess_html (see http://drupal.org/node/1354#functions)
• function min_preprocess_html(&$vars) {
• html.tpl.php
• File: @file block missing (see http://drupal.org/node/1354#files)

> Removed my specific git clone command from project page.

But we need a generic git clone command in the summary please!

Added a working git clone command to the summary.

(It looks like many applicants are completely incapable of reading and following instructions about the use of git, and it is quicker to fix these, instead of trying to teach them how to do it right.)

> It looks like many applicants are completely incapable of reading and following instructions about the use of git

I see it as a test ;)

- Screenshot is very nice in a minimalistic way, but doesn't follow the guidelines for theme screenshots.
- the favicon doesn't work
- CODE elements aren't styled, and so are dramatically smaller than body text
- needs some space at the bottom of the page -- my footer block is right at the bottom
- node tabs are a bit wonky -- there's a tiny space between the line and the tab, and also they're very close to the node content, given the size body and title fonts in this theme
- formatting tips on comment forms are broken. The "More information about text formats" line is HUGE and the list of formatting rules doesn't have bullets
- "Enter a comma-separated list of words to describe your content" is all in caps, which is weird.
- various other field widgets are a bit funny
- there's no template.php file. This is perhaps not a requirement... but it seems a bit odd not having one.

 * Default theme implementation for Drupal Comments.

Not true! It's Min's, not the default!

        <!-- START Google Analytics: change UA-XXXXX-X to be your site's ID. -->
        <script>
            (function(b,o,i,l,e,r){b.GoogleAnalyticsObject=l;b[l]||(b[l]=
            function(){(b[l].q=b[l].q||[]).push(arguments)});b[l].l=+new Date;
            e=o.createElement(i);r=o.getElementsByTagName(i)[0];
            e.src='//www.google-analytics.com/analytics.js';
            r.parentNode.insertBefore(e,r)}(window,document,'script','ga'));
            ga('create','UA-XXXXX-X');ga('send','pageview');
        </script>
        <!-- END Google Analytics: change UA-XXXXX-X to be your site's ID. -->

This should be left to the Google Analytics module.

- README.md

The standard for Drupal READMEs is to have the filename README.txt. No harm in using markdown inside it though :)

> The Date module is not required, but if you want dynamic
date fields it is recommended:
* Date (https://www.drupal.org/project/date)

I can't see any calls to Date module functions...

- The indentation in html.tpl is wonky.

Overall, this looks ok. I find the capitals a bit OTT and I find they clash with the site title, but that's just a matter of taste :) What would be really nice would be support for Color module.

PS. One more thing: lack of sidebars is a going to be a dealbreaker for a lot of users.

@joachim: one correction: the standard for Drupal projects is to use README.md or README.txt. See https://www.drupal.org/node/161085

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Automated Review

No problems: http://pareview.sh/pareview/httpgitdrupalorgsandboxc-whitman2157739git

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Coding style & Drupal API usage

Just some recommendation:

1. this is a responsive theme, but all menu links and inline links are very small, smartphone users see a very big text in all the contents and very small links, you should consider to size tap targets appropriately
2. In admin/appearance/settings/min "site slogan" option have no effect, please look in your page.tpl.php
3. Same thing for "Secondary menu" option

nice theme @chadwhitman.. "just ..recommendation" alone does not justify this to be moved back to "Needs work".. I would recommend RTBC unless someone want to have a final look at this..!

Hello Binu Varghese,
In the administration of this theme (admin/appearance/settings/min) there are two options but these options don't work because in the page.tpl there are not the corrspective functions: do you think that this thing do not need work?

@Skin, you are right! @chadwhitman please fix this.

Thank you for your contribution - I like the cleanless and simplicity of your theme and actually also consider using it with my own blog :)

There are some issues, mostly related to coding style. I do not see any reason not to promote this to full project. Please chekc out these suggestions and do correct some minor things before you create a 7.x-1.0 -release.

Automated Review

No problems found with pareview.sh / drupalcs / coder.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Do not apply for themes.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

I think yes: Follows the guidelines for project length and complexity. Theme does not have that much code, but this theme has a few tempalates and Sass support.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order:

1. (+) Date -module is mentioned as required module, however there are no Date-functions in use. In the "CONFIGURATION" -section there are some information of Date -field usage, if it is in use. I suggest you remove Date as a required module since it does not seem to be required but only suggested.
2. Sass configuration file config.rb
   • contains extensions_dir/images_dir/javascript_di variables. However there are no such directories.
   • Sass environment -variable would perhaps better be ":production" (and CSS recompiled)
3. README.md still contains information related to removed Google Analytics -code, see comment #40, please remove (and I'd recommend not to instruct users to modify theme files by hand in the first place)
4. html.tpl.php: DOCTYPE should be on the 1st line (ie. no empty line above it), see http://www.w3.org/TR/html-markup/documents.html#conformant-documents
   

   ...*/
   ?><!DOCTYPE html>
   <!--[if lt IE 7]>...

5. html.tpl.php: does not follow recommended coding style (code indenting varies line by line), see https://www.drupal.org/node/1965
6. (+)? page.tpl.php: Are you sure you want to print #logo and #site-info -divs even when there would not be content? They take space (margin-top, padding-right => 25 x 10 px) according to your Sass components/_images.scss:16. I suggest moving if-statement outside
   
   and
   

   containers instead of inside them.
7. consider using , and -tags to wrap different sections on your theme, quite like you are wrapping header and footer atm. I guess footer would also be better off after div#page, not inside of it?
8. consider wrapping $secondary_menu inside footer with a of
   
   or alike to ease styling (not menu is plain ul-list, and is not wrapped with tag & class or id)
9. consider changing font size even smaller in narrow displays (currently 25px, perhaps 20px would be better?) as used font is quite large by itself - smaller screens can't fit that much text with this size of a font.
10. template.php is not present, but it is not required - however you have removed your template.php and again referencing Google Fonts directly in html.tpl.php (multiple commits 17.8.2014, emptied template.php removed with commit 4c09b80266)

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.