[D7] Insha

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxManjitSingh2245863git

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.

After enabling this theme there are a few script errors, likely due to the first error which is a missing JS library.

Failed to load resource: the server responded with a status of 404 (Not Found) http://example.com/sites/example.com/themes/insha/libraries/selectbox/js/jquery.selectbox-0.2.js?n58iz8
Failed to load resource: the server responded with a status of 404 (Not Found) http://example.com/js/jquery.selectbox-0.2.js?n58iz8
Uncaught TypeError: undefined is not a function layout.js:2

I also think js/layout.js might need to be wrapped like this:

  (function ($) {
    // YOUR CODE HERE
  })(jQuery);

I've added the selectbox library and it worked, however, there is still an issue with the library. It appears to be looking for it in two places, it worked after I placed the library in both of these locations: sites/all/libraries/selectbox AND sites/example.com/themes/insha/libraries/selectbox

When my browser (current version of Chrome) is between 768px and 846px window width the content column falls awkwardly below the left sidebar. https://www.dropbox.com/s/hiwzkfse6kutkpw/Screen%20Shot%202014-05-13%20a...

Not sure if this is intentional, the search magnifying glass falls below the text area instead of being inline. (See above screen shot)

Also not sure if this in intentional, the footer columns don't stack when in a mobile layout, instead the four columns squish together. https://www.dropbox.com/s/6ja9seho63lke4x/Screen%20Shot%202014-05-13%20a...

Good looks to me.
But I do not mind if you make very small changes.

Hello, Manjit.Singh.
This works well for me.

I do not insist, but suggest.

- template.php
insha_breadcrumb()
use theme('item_list', array()) instead ol li.

- layout.css
font-family: 'Glyphicons'; replace to font-family: Glyphicons, sans-serif;;
border: 0px; replace to border: none;;
padding: 0px 40px 0px; replace to padding: 0 40px 0;;
to remove padding: 0; in ul.primary li a { (it overwriten below);
left: 0%; replace to left: 0;;

- comment-wrapper.tpl.php
tag of class="title-comment", element div is not allowed there. You can use span.

- README.txt
please rename "Dependecies" to "Dependencies".

Thanks.

description = Custom theme

Should say something more descriptive!

return '<ul class="margin0 menu padding0">' . $variables['tree'] . '</ul>';

Wouldn't it be better to use semantic classes here?

'user' => 'secnav_account',

Class name seems a little odd, when surely you can use a parent selector for the menu it's in?

  drupal_add_css(libraries_get_path('selectbox') . '/css/jquery.selectbox.css', array('group' => CSS_THEME, 'every_page' => TRUE));
  drupal_add_js(libraries_get_path('selectbox') . '/js/jquery.selectbox-0.2.js', array('group' => JS_THEME, 'every_page' => TRUE));

IIRC Libraries module now has support for themes, so it would be better to use that rather than load it here.

Theme only has one sidebar, but it's called 'sidebar first'.

When the window gets narrow, the sidebar is shown above the content which seems bit odd.

Theme states that jQuery Selectbox is a dependency, but the page for that at https://code.google.com/p/select-box/ says:

> jQuery Selectbox plugin replace browser built-in select box form element with fancy high customizable one.

I don't think it's necessarily a good idea to bundle UI improvements like that in a theme.

A few things in forms look odd, as you can see in this screenshot of the theme settings.

hi,
Thanks for the contribution.
The project looks promising :)

here is my review :
- the theme has a templates folder but html.tpl.php is out if the folder.
- maybe it's a good idea to add to Readme file that some font are requested from http://fonts.googleapis.com (line 17 html.tpl.php)
- template.php is full of hard coded ul, li please use theme('item_list', $variables); for those.
- template.php hard codes h2 please use theme('html_tag', $variables); for those.
- template.php hard codes a for links, please user l(); for those.
- function comments are not well formed in template.php per example, please have a look at example here : https://drupal.org/coding-standards/docs
- also the automatic review finds some issues to be fixed (adding them here as attached file)

Thanks again

Automated Review

PAReview came up clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code/content

No:

The bundled font (Glyphicons) appears to be third party code/content. Third party code/assets is not generally allowed on Drupal.org and should be deleted.
This particular asset is made available under the Creative Commons Attribution license. This also violates policy, since all assets hosted on Drupal.org must be licensed under the GPL V2+ license.
This policy is described in the getting involved handbook. 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.

Project page

No. Please take a moment to make your project page follow tips for a great project page. In particular, you must add an "Requirements" section. with a link to required modules and any other extraordinary items or steps needed to install or use. Mention and link to all required external libraries.

README.txt

No.

To be functional it is necessary to download and install the third-party select-box library and install and enable the libraries module. Such dependencies must be stated in README.txt

There also nothing in the README.txt about configuring the theme. For instance: How does one toggle between the 1-column and 2-columns layout?

To also need to make sure your README.txt follow the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Postponed until the documentation issues are sorted out.

Coding style & Drupal API usage

The line dependencies[] = libraries is missing from insha.info.
Code walk-thrugh is postponed until the documentation issues are sorted out.

Setting back to "Needs work", since none of issues of review in #23 is resolved.

Hey @Manjit.Singh,

Looks like you are still calling selectbox in your .info file:

stylesheets[all][] = libraries/selectbox/css/jquery.selectbox.css
scripts[] = libraries/selectbox/js/jquery.selectbox-0.2.js

Also in your layout.js which is causing console errors:

TypeError: jQuery(...).selectbox is not a function
  jQuery("select").selectbox();

I would recommend adding the jQuery Selectbox module to your recommendations on your project page and readme rather than trying to include it yourself.

This is minor, but you probably want to add a margin-right to your input buttons:

Your README.txt file is still incomplete. You need to follow the README.txt template.

I think you're getting close! Hopefully you'll get approval soon.

Automated Review

Best practice issues identified by pareview.sh seems to be clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code/content

No: Please follow the guidelines for 3rd party code.

Manjit, can you please tell us in which Bootstrap theme you've found that they have directly imported the glyphicons fonts directly into their theme directory? What I have found is they have used this below mentioned script in Bootstrap 3 if possible please try to find out how these people have achieved the same in Bootstrap 2 cause I've found that they have used the code in CSS but I was unable to find those respective 'glyphicons' files.

/**
 * @file
 * icon.vars.php
 */

/**
 * Implements hook_preprocess_icon().
 *
 * Bootstrap requires an additional "glyphicon" class for all icons.
 *
 * @see icon_preprocess_icon_image()
 * @see template_preprocess_icon()
 */
function bootstrap_preprocess_icon(&$variables) {
  $bundle = &$variables['bundle'];
  if ($bundle['provider'] === 'bootstrap') {
    $variables['attributes']['class'][] = 'glyphicon';
  }
}

README.txt/README.md

No: Does not follows the guidelines for in-project documentation.

In the 'Installation' you've mentioned that "You can find theme on http://drupal.org/project/insha." which is not valid! Please fix this.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

I'm attaching you a screenshot, this is what happened when my user is having its profile picture. It really needs to be fixed.

Some minor things which I've noticed:

1. Please check in your .info file first line is empty.
2. It would be good enough if you can add something like this ; SETTINGS before defining your theme settings in your .info file.

Thanks for contributing to Drupal!

This review uses the Project Application Review Template.

In the 'Installation' you've mentioned that "You can find theme on http://drupal.org/project/insha." which is not valid! Please fix this.

The simplest fix is just to delete it. Anyone that has the README file as already found and downloaded the theme, so there is no need to tell people where to find it in this file.

Please correct me if i am wrong in case of Glyphicons.

You're wrong. Inclusion of the Glyphicons in your repo violates Drupal.org policy.

Third party code/assets is not generally allowed on Drupal.org and should be deleted.
This particular asset is made available under the Creative Commons Attribution license. This also violates policy, since all assets hosted on Drupal.org must be licensed under the GPL V2+ license. See https://www.drupal.org/node/422996 and https://www.drupal.org/node/1001544.

You point out that Bootstrap Business also violates this policy. That seems to be the case. However, that there exists license violations in the repo already does not mean that it is OK to add another license violation.

Please take a look at Bootstrap. This theme also allow the use of Glyphicons, but does not include them in the repo. Instead, the site builder is instructed to download them directly from the Bootstrap website. This is the accepted method for combining third party materials with a theme, and the one you should use.

Removed links that are not project application reviews.

Looks like you forgot to add the review bonus tag after you did the reviews? Adding it now.

So this blocked on the included third party glyphicons right now. Please remove them and tell users where to download them.

Hi Manjit Singh,

Nice theme, Simple & clean.
- Copyright text is missing - Add Copyright with website name & year.
- Reset HTML body style - You should reset body{ margin:0; padding:0; } else browser's add default spacing based on browser standards. If you want to add any margin or padding add in your stylesheet so that it will perform same in all browsers.
- Display error above the form fields (Attached screenshot)
- Article footer empty space - footer should not display if there is no content. attached screenshot

Thanks

Automated Review

The automated review of your project by PAReview has found some issues with your code. As coding standards make sure projects are coded in a consistent style we ask you to please have a look at the report and try to fix them.

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 code/content

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

(Maintainer has replaced Glyphicons with own work.)

README.txt

The instructions for enabling the theme is: "Go to administer > site building > themes. Check the 'Enabled' box next to the theme.".
However, the correct breadcrumb seems to be "Home » Administration » Appearance", and there is no checkbox there, but a link: "Enable and set default". Easy enough to figure out if you know Drupal, but you may want to fix this to avoid confusing new users.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

The line scripts[] = js/layout.js is in insha.info, but the script is not in distro, producing a warning. Theme seems to work fine without it, but you should remove the reference to silence the warning, or add the file to the repo if it is supposed to be part of the theme.

AFAIK there is no requirements for themes to have a copyright text, so I do not agree with "Copyright text is missing" in #36. Any site builder who want a copyright text can add it him-/herself.
I don't regard any of my findings as blocking issues, so I am moving it to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyeballs.

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.

Code looks good to me.

Thanks for your contribution, Manjit.Singh!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.