[D7] Bootpress

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2338365

Project 2: https://www.drupal.org/node/2308975

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Timeout when invoking pareview.sh for http://git.drupal.org/sandbox/pitabas/2338347.git at http://pareview.sh/pareview/httpgitdrupalorgsandboxpitabas2338347git

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.

Automated Review

1. Git default branch is not set, see the documentation on setting a default branch.
2. Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
3. Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting

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

No: Does not follow the licensing requirements.

3rd party assets/code

No: Does not follow 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.

Secure code

No: List of security issues identified.

(*) Theme have XSS bug in Header Message, Copyright Info and Social links fields. Details screenshot attached.

Coding style & Drupal API usage

1. (*) Theme info file have not this line core = 7.x. Whats why I can't able to enable the theme. Ref boot-press1.jpg
2. (*) There is a option to enable or disable the theme jQuery. But if I disable theme jQuery then getting js error and many options not working. There should be fallback codes to handle the js codes or a message about it. ref boot-press2.jpg
3. (*) Social links icons have dependency with Fontawesome, so id Fontawesome disable then icons not showing. Need the mention about it or need fallback system.
4. (*) Some of your code have dependency of bootstrap.js. So, if I disable it, the mobile pull down menu not working.
5. (*)
   

   <script type="text/javascript">
       var ENABLE_STICKY_NAVIGATION = "<?php echo (theme_get_setting('sticky_main_navigation')) ? 1 : 0 ; ?>";
       var ENABLE_DEMO_CAROUSEL = parseInt("<?php echo (theme_get_setting('demo_carousel')) ? 1 : 0 ; ?>");
     </script>

   These lines in html.tpl.php and this should be go to the drupal settings vars.

6. (+) Bootstrap carousel only for demonstration so it should not part of header.tpl.php
7. Push theme settings to top of all settings. Ref boot-press4.jpg
8. Git clone url is wrong. Should be
   

   git clone --branch 7.x-1.x http://git.drupal.org/sandbox/pitabas/2338347.git boot_press
   cd boot_press

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.

Hi,

Nice theme,
Few suggestions and bug.
1) Slider navigation is not working
2) Empty space - block/region container should not visible if there is no content insider.
3) Dropdown is not working - created multi-level menus and enabled " Show as expanded" still not working dropdown menu.

Attached all screenshots.

Thanks

Hello Pitabas ,
thanks for this theme, it looks very promising.

Git Clone Command

you submitted your personal git clone command, when I try to download the theme I'm asked for a password, please consider adding a working git clone command to the summary .

Automated Review

Best practice issues identified by pareview.sh please see http://pareview.sh/pareview/httpgitdrupalorgsandboxpitabas2338347git

licensing

This project contains the Font Awesome font which is licensed as a "GPL Friendly", but not GPL 2 or later compatible license.

3rd party assets/code

• Drupal bootstrap (https://www.drupal.org/project/bootstrap)
• font awesome ( https://www.drupal.org/project/fontawesome )
• jQuery (use https://www.drupal.org/project/jquery_update)

Some of these libraries need to use sites/all/libraries and be maintained outside of the theme.

The search form is not displayed proprely on the search page.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code. Usually third part library (FontAwesome, jQuery) are not packaged with theme. You should rely on existing module and display warning when they are not installed or activated.
For example, If your theme need jQuery 1.10, you can add this code into your theme settings :

  <?php
  	$jquery_version = variable_get('jquery_update_jquery_version', '1.5');

    if (!module_exists('jquery_update') || !version_compare($jquery_version, '1.10', '>=')) {
      drupal_set_message(t('!module was not found, or your version of jQuery does not meet the minimum version requirement. Boot Press requires jQuery 1.10 or higher. Please install !module, or Boot Press may not work correctly.',
        array(
          '!module' => l('jQuery Update', 'https://drupal.org/project/jquery_update', array('external' => TRUE, 'attributes' => array('target' => '_blank'))),
        )
      ), 'warning', FALSE);
    }
    ?>
  

You can apply the same process by using Bootstrap and Font Awesome Icons modules and load them via Libraries API

Coding style & Drupal API usage

1. (+) The caroussel Demo is not customizable and should be removed. People can easily create their own caroussel with contributed module like Views Slideshow

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.

Hi Pitabas,

1. I get this error : Fatal error: Call to undefined function libraries_detect() in /path/to/drupal/sites/all/themes/boot_press/template.php on line 163
   You should check this statement first

   <?php
   if (module_exists('libraries') && function_exists('libraries_detect')) {
   // Your code here
   }
   ?>

2. The link title on Bootstrap message is not correct. Replace "jQuery Update" with "Bootstrap Library"

The rest looks good.

Also the Automated Review has some issues:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpitabas2338347git

Automated Review

Their are issues at: http://pareview.sh/pareview/httpgitdrupalorgsandboxpitabas2338347git

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.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Add Dependancy to https://www.drupal.org/project/libraries as i was getting error (Fatal error: Call to undefined function libraries_detect() in /path/to/drupal/sites/all/themes/boot_press/template.php on line 163) once theme enabled.
2. Header Message is not clearly visible

Else everything looks okay.

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.

Hi Pitabas,

Yes, that was the problem with jQuery library missing.
Can you list required module details in issue/project page description.

Thanks

Automated Review

There is an issue: Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting.

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. However, you include three javascript libraries in js/lib that are licenced under the MIT license. I would suggest using bower or a make file to add this javascript files. What about the images in images/carousel? Are you allowed to redistribute them under GPL?

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. The following functions in template.php are lacking of Doxygen style comments.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements. Found no issues.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. Typos in requirement messages after activating the theme: "moduel was not found".
2. (*) Move all preparing of your variables in hook_preprocess. See for example comment.tpl.php.
3. (*) Don't hardcode js files in html.tpl.php. Use drupal_add_js() in hook_preprocess.
4. Use drupal_attributes to set your classes (e.g. for the socialmedia buttons).
5. (*) You use preg_replace to get the user picture url in _boot_press_create_user_picture_url() in template.php. Don't do this, you can use just print $variables['picture']

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.

Although there are some issues the theme looks nice and can be a great contrib project.

This review uses the Project Application Review Template.

Setting status to needs work.

Hi Pitabas!
In templates you shouldn't have processing code. Variables should be defined or adapted in the relevant preprocess functions.
In comment.tpl.php for example you need to move the following block to preprocess:

<?php
  global $base_url;
  $comment_user = user_load($comment->uid);
  $comment_post_date = format_date($comment->created, 'custom', 'F jS, Y  g:i a');
  $comment_path_alias = drupal_get_path_alias('user/' . $comment_user->uid);
  $comment_author_link = url("", array('absolute' => true)) . $comment_path_alias;
?>

BTW: In this case it may be also better to prepare a $bootpress_gravatar variable already including all html.
And I think you can use something like this (not tested):

 $bootpress_gravatar = l($gravatar_image_html, 'user/' . $comment->uid, array('html' => TRUE));

Hi Pitabas,
please check also the other templates. And there are still some coding style issues: http://pareview.sh/pareview/httpgitdrupalorgsandboxpitabas2338347git

And for your custom javascript in main.js please have a look at https://www.drupal.org/node/756722 (see especially the drupal.behaviours section).

Hi Pitabas,

Just read the documentation about using JQuery and Behaviours in Drupal. It should look something like this:

(function ($) {
  Drupal.behaviors.exampleModule = {
    attach: function (context, settings) {
      $('.example', context).click(function () {
        $(this).next('ul').toggle('show');
      });
    }
  };
})(jQuery);

This is all in the documentation link I posted above. Also there is a processing php block in social-media.tpl.php, that should be moved to preprocess.

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 :-)

Hi Pitabas,

Great, there are some improvements visible. :)

But why don't you prepare a variable for the socialmedia links in boot_press_preprocess_page() using _boot_press_social_media_links()?
You shouldn't use a helper function in a template, just print variables.

I also found another small issue in boot_press_preprocess_html(): Don't use the alternate control statement syntax using : instead of brackets. This is only allowed in templates. The following code needs to be fixed (please check also all your other code):

<?php
if (theme_get_setting('responsive_respond', 'boot_press')):
    $respondjs = array(
      '#tag' => 'script',
      '#attributes' => array( 
        'src' => $path_to_theme . '/js/libs/respond.min.js', 
      ),
      '#prefix' => '<!--[if lte IE 9]>',
      '#suffix' => '</script><![endif]-->',
    );
    drupal_add_html_head($respondjs, 'respond');
  endif;
?>

And always add linebreak after a bracket and before elseif or else. The following code needs to be fixed (please check also all your other code, there is also a missing space after the if):

<?php
if($variables['page']['sidebar_first'] && $variables['page']['sidebar_second']) { 
    $variables['sidebar_grid_class'] = 'col-md-3';
    $variables['main_grid_class'] = 'col-md-6';
  } elseif ($variables['page']['sidebar_first'] || $variables['page']['sidebar_second']) {
    $variables['sidebar_grid_class'] = 'col-md-4';
    $variables['main_grid_class'] = 'col-md-8';		
  } else {
    $variables['main_grid_class'] = 'col-md-12';			
  }
?>

The js in main.js is better now, but still missing the Drupal Behaviours!

(function ($) {
  Drupal.behaviors.boot_press = {
    attach: function (context, settings) {
      //YOUR JS
    }
  };
})(jQuery);

Sorry for the delay, but had just another look on your theme.

Looks very good now, from my side only the following is left:

• Two functions in template.php are lacking of Doxygen style comments (the documentation block).
• And if you don't display the slider I would delete the carousel images (copyright issue?).

Okay, looks good for me now.

To speed up the process I recommend doing 3 manual reviews to get a review bonus: https://www.drupal.org/node/1975228

Thanks Pitabas for Contribution, Nice theme.

Done with Manual review locally, and found some UI bugs. Please find below.

1. Bulleted texts are not align with the title. check bullet's_text.png
2. If user login form is in banner region, there is no spacing at left and right side. Please check margin-left-right.png
3. Please remove border bottom of last child, It would be good for UI perspective. check menu_border.png
4. Page title collapse with breadcrumbs. check page_title.png
5. Check spacing.png, No spacing b/w navigation and search bar.

That sound like possible improvements but not application blockers, anything else that you found or should this stay RTBC?

@klausi stay it in RTBC, I have not found any blocker as of now.

Thanks for your contribution!

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.