.

CommentFileSizeAuthor
screenshot.png41.94 KBshafiqissani
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shafiqissani’s picture

Project: Drupal.org security advisory coverage applications »
Component: theme » Code
shafiqissani’s picture

Project: » Drupal.org security advisory coverage applications
Component: Code » theme
PA robot’s picture

Status: Needs review » Needs work

Link to the project page and git clone command are missing in the issue summary, please add them.

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.

shafiqissani’s picture

Status: Needs work » Needs review
PA robot’s picture

Status: Needs review » Needs work

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

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

shafiqissani’s picture

Status: Needs work » Needs review
artem_sylchuk’s picture

Status: Needs review » Needs work
Major coding standards / best practice issues
An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

You can find the results of the automated report at http://pareview.sh/pareview/httpgitdrupalorgsandboxshafiqissani2078519git.

Also

PAReview: 3rd party code
modernizr-2.6.2.min.js appears to be 3rd party code. 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, 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.

Also you probably will have problems with custom jquery included into your theme. Please consider this manual: https://drupal.org/node/1058168

shafiqissani’s picture

Status: Needs work » Needs review
shafiqissani’s picture

fixed

aramboyajyan’s picture

Status: Needs review » Needs work

Overview

It is not clear what issues does this theme solve/provide. Explain please how is it different that two other existing H5BP themes?

Code

  • remove one indentation to all code placed in template.php
  • you can load modernizr through the Modernizr module that utilizes Libraries API
  • there is unnecessary white space in many files - code formatting is not per Drupal standards
  • hyperlink reference for Apple touch icons is not done properly and results in 404 errors;
    use href="<?php print path_to_theme() . base_path(); ?>/img/touch/[...]" instead of href="img/touch[...]"
  • stay consistent with conditionals in template files:
    use if ($tabs) instead of if ($tabs = render($tabs))
    see core themes for guideline, e.g. Bartik
  • also, whenever you include CDN that supports SSL, you should not state the protocol directly, so the browser can load the script both on "http" and "httpS" pages automatically; example: instead of http://code.jquery.com/[...] you should use //code.jquery.com/[...]
  • comments should end with dots and be formatted per Drupal guidelines
  • remove jQuery v2.x as it is completely unnecessary; it breaks Drupal's JS because it is not compatible with jQuery v2.x. Current version included in the core is v1.4.4 (D7) which can be upgraded using jQuery update module
  • other included scripts are just pasted from H5BP; see Working with JavaScript and jQuery for more information

See pareview.sh results for more issues. Also before submitting your theme make sure that you check it with Coder module. Right not it returns 95 normal warnings.

Git

No track of the history. There are four commits in total, out of which 2 are "Initial commits".
I suggest re-creating the history and saving the progress step by step, as that will show others what your commit-style is and that you are familiar with using Git for regular tasks.

Bottom line

Unfortunately, too many technical mistakes. I believe it would be better to postpone the development of this theme.
Add more new features so it is different than two existing ones and improve the knowledge of Drupal coding.

This should be confirmed by other members. Changing status to "Needs work".

@shafiqissani let me know if you have any comments.

kscheirer’s picture

Title: D7 Mobile boilerplate » [D7] Mobile boilerplate
  • You have a lot of code style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxshafiqissani2078519git, especially in template.php please resolve those.
  • Do you still need main.js?
  • In helper.js, you should use the Drupal. namespace, or Drupal.settings.MBP for your custom settings
  • I'm not sure about adding another jquery library at with drupal_add_js('http://code.jquery.com/jquery-2.0.3.min.js', 'external');, Drupal already comes with jQuery. Won't loading 2 versions of the same library cause problems?

----
Top Shelf Modules - Crafted, Curated, Contributed.

kscheirer’s picture

Issue summary: View changes

added link and drupal version.

shafiqissani’s picture

Priority: Normal » Minor
Status: Needs work » Closed (duplicate)
shafiqissani’s picture

Issue summary: View changes

.

PA robot’s picture

Issue summary: View changes
Multiple Applications
It appears that there have been multiple project applications opened under your username:

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

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

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.

apaderno’s picture

Related issues: +#2127077: [D7] Varnish All