[D7] BlueJems

Hi,

There are many errors to correct. See report on http://ventral.org/pareview/httpgitdrupalorgsandboxigoen1513454git

These are errors of styles, you must respect the standarts Drupal in comments, indentation, ...

For example you use 'tabs' while only spaces are allowed to indent the code, the old CVS tags ($ Id) is no longer needed, ...

In addition, you must add a README.txt and remove LICENSE.txt which is added automatically by drupal.

These are not necessarily errors that prevent the smooth functioning of the theme but the rules to ensure a good reading source code for the entire Drupal community.

Hi,

I'm unable to clone the project (igoen@git.drupal.org:sandbox/igoen/1524924.git), it is asking for password!!

nalan,
maybe he means by:
git clone http://git.drupal.org/sandbox/igoen/1524924.git bluejems

Hi Dako,
I tried it, but it clones only 3 files (2 info files and one readme.txt).
I don't know what is going wrong!!
Have you cloned it? Is it cloning well?

Nalan
I think that it is master brache http://git.drupal.org/sandbox/igoen/1524924.git which should contain only readme.txt
git clone --branch 6.x http://git.drupal.org/sandbox/igoen/1524924.git bluejems
and git clone --branch 7.x http://git.drupal.org/sandbox/igoen/1524924.git bluejems

igoen please read
Release naming conventions
Moving from a master to a major version branch
Creating a branch or tag in Git
and correct your project.

Thanks for the info Dako.

Spam reported, sorry guys
#1525562: Spamer: hammsa.com

Hi igoen,
I just checked your theme, it looks good, especially the “highlighted” and “content-top” regions are well designed. Here are my manual reviews:

Manual review:
1) The sidebar-left and sidebar-right are not working. I created new blocks and placed them in left and right sidebars, they are not working. But the same blocks work fine in the bottom regions.
2) If we place the search form in the left sidebar, the design breaks completely (see screenshot in the attachment)
3) The photos that you have used in slideshow, do you have proper rights to distribute it on drupal.org under the GPL?
4) Remove the license file (it will be added by drupal team) and add a well documented readme file. Also please mention how to use the slideshow option.
5) Put the template files (ie: *.tpl.php) in a separate folder called “templates” and put the css files in a separate folder called “styles”. By doing so, you need to change the path in the info file too. This suggestion is just to make the theme well structured.

update

Hi,
1) I cloned your latest version from “ git clone --branch 7.x-1.x http://igoen@git.drupal.org/sandbox/igoen/1526544.git bluejems” and checked it. Regarding the sidebars (left and right), if you don’t need it, then you can remove it from the info file. In that way, it cannot be seen in the “Regions” (…/#overlay=admin/structure/block). Then, unlike you said, you have not disabled the left sidebar in page.tpl.php but you put them in comments and those comments are only for html tags and it won’t work for php codes. Could you check if the default sidebar is working?

2) For the design break, if you are going to remove the left sidebar then you don’t have to worry about it. But check if is displaying properly in default right sidebar.

Hi Igoen,
There are lot of coding style errors that you have to take care of which I found through auto review http://ventral.org/pareview (see attachment drupalcs-result.txt).

Here are my manual reviews for --branch 7.x-1.x,
1) The design of the sidebars (left and right) is slightly collapsed in firfox3.5 and in IE8 the left sidebar is completely missing (see attachment screen1_firefox3.5.png and screen1_IE8.png).
2) Add README file.
3) Put the template files (ie: *.tpl.php) in a separate folder called “templates” and put the css files in a separate folder called “styles”. By doing so, you need to change the path in the info file too. It is not a good idea to put template files and css files in the same place.

update

Hi Igoen,

It wil take some time to correct the coding styles, take your time. Let me know if you need any help to correct some style issues.

1) Good.
2) The README.txt should contain

• Brief description about your theme
• Requirements
• How to install
• etc....

This link may help you to create the read me file guidelines for in-project documentation. You can also check the readme file from other popular themes and analyse how they have structured and provided the information in the readme file.
3) If you replace the CSS files in new folder "styles", you have change its path in the info file (stylesheets[all][] = styles/style.css). If you replace the *.tpl.php files in new folder "templates", you don't have to do anything, drupal will take care of it automatically. It's simple, isn't it!!

Hi,

The branch 7.x-1.2 is not found.
Could you give me the direct clone link for the right branch.

Could you attach the errors which you are in need of help.

Hi igoen,

1. I think the branch should be 7.x-1.x

2. The folder 7.x-1.0 shouldn't exist, the files should be in the root of the repo.

3. I think you shouldn't prescribe which blocks are where, specifically 'authorize' should be configurable, what's wrong with having the drupal login block positioned somewhere? I'd get rid of authorize block.

4. What is

  function bluejems_page_class($sidebar_first, $sidebar_second) {
    if ($sidebar_first && $sidebar_second) {
      $id = 'layout-type-2';
    }
    elseif ($sidebar_first || $sidebar_second) {
      $id = 'layout-type-1';
    }

    if (isset($id)) {
      print ' id="' . $id . '"';
    }
  }

Doing? The print seems wrong?

5. Also, lots of coding style problems, spaces indenting, etc. in template.php

I think pareview/ Drupal Code Sniffer is freaking out as it shows no errors but I see lots of whitespace and indenting issues in page.tpl.php

6. styles/style.css line 6 should be
font-family: Arial, Verdana, sans-serif;

7. There is a CSS problem with the authorise block: - see attached image, mac chrome and FF latest builds.

I think it needs a bit more work but it's a promising and good looking theme - good luck!

update

I'm sorry, there are currently hundreds applications waiting for a review and only a handful of reviewers.
We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

Please see the issue priority guidelines: http://drupal.org/node/894256#app-priority

Your forgot to add the "PAReview: review bonus" tag as outlined in #1410826: [META] Review bonus.

Hello. Nice and clean looking theme, and lightweight code. Most stuff seems to work great, but I found these small issues:

1. Your README in the master branch should only contain instructions about using the right branch.

2. The navigation block half-way disappears when used out of the box (see screenshot). I am on ubuntu 12.04 chrome.

3. Also regards the login and register links, the same (in that region).

4. The comment form subject is out of bounds. see screenshot.

5. Also, as someone mentioned. There is no documentation on how to customize the slideshow.

6. There is also no documentation on how to use the "superfish menu" region. I can see from the template that it never gets rendered if you have a main menu. So I tried to delete all links, and the attached screenshot is the region with the menu "development" (from devel module).

Minor:
- titles with long words are not wrapped in second sidebar. see screenshot.

Atomated review:
Remove "version" from the info file, it will be added by drupal.org packaging automatically.

Good job, and good luck!

update

Thanks for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

manual review:

1. Please take a moment to make your project page follow tips for a great project page.
2. Manual Guide.txt: That should go into README.txt.
3. jquery.cycle.all.js and other JS files: 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.
4. bluejems_preprocess_html(): this is a hook implementation and should be documented as such, see http://drupal.org/node/1354#hookimpl . Same for bluejems_preprocess_maintenance_page().
5. template.php: do not execute code in the global scope. The check for the front page to add some Javascript belongs into a function definition (hook implementation).
6. page.tpl.php: $user->name: Do not print the username like that, as it is user provided input. Use theme('username', ...) and it will generate the link markup for you.
7. page.tpl.php: all user facing text ("logged in as") must run through t() for translation. And don't forget to use placeholders for dynamic values like the username.
8. page.tpl.php: all style information should be defined in CSS files.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

fixed sandbox link

manual review:

1. Please take a moment to make your project page follow tips for a great project page.
2. t('logged in as ') . ' <a href="' . url('user/' . $user->uid)...: do not concatenate translatable strings, use placeholders instead with t(), e.g. t('logged in as !user', array('!user' => ...)) Also elsewhere.
3. ' <a href="' . url('user/' . $user->uid)...: do not create link markup yourself, use l() instead. Also in other places.
4. maintenance-page.tpl.php: style information should go into a CSS file.

Although you should definitively fix those issues they are no application blockers, otherwise RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

updates

Please make your project page (--> http://drupal.org/sandbox/igoen/1586444) follow the tips for a great project page (not this issue summary ;) )

Thanks for your contribution and welcome to the community of project contributors on drupal.org! :)

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.