[D7] Royal Olive

Multiple Applications

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

Project 1: https://drupal.org/node/2119647

Project 2: https://drupal.org/node/2091715

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.

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

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.

Multiple Applications with [D7] Commerce HDFC Payment Gateway.. So closing this one for the time being and reopening the other..

Added links to demo site and sandbox page

@Vinay Punyamurthy

I was just testing your code on pareview.sh and there were whole lot of issues.
Refer http://pareview.sh/pareview/httpgitdrupalorgsandboxpvinayv2119559git for the list of issues.

Try to improvise it but removing all the issues.

Two things with regards to branches.

There is still a master branch. See: https://drupal.org/empty-git-master

The working branch should not be called royal_olive_branch, but probably 7.x-1.x. See: https://drupal.org/node/1015226

Here is the updated report status: http://pareview.sh/pareview/httpgitdrupalorgsandboxpvinayv2119559git

I only have one issue with regards to a manual check of the code:

1. initialize.js and slide.js should use Drupal behaviours. See: https://drupal.org/node/171213

(function($) {
Drupal.behaviors.myBehavior = {
  attach: function (context, settings) {
    //code starts
    $("body").click(function() {
      alert("Hello World");
    });
    //code ends
  }
};
})(jQuery);

EDIT: Removed my comment about 3rd party code, as I realised that GPL licensed code is allowed.

Just a few more comments and recommendations:

1. Instead of "myNAME" for the JS Drupal behaviours, I'd suggest adopting a naming convention like "royal_olive_initialize", "RoyalOliveInitialize", or something similar that contains your theme namespace to ensure you won't clash with any other module or theme.

2. Consider removing the extra single space at the end of L4 in template.php, as some editors (like mine) complain about there being white-space at the end of a line which is generally not allowed. (Yes, I know it's in a comment block, hence just a suggestion.)

3. Not all your hooks and theme implementations in template.php have the "Implements hook_NAME()" or "Overrides theme_NAME()" comment above them, I'd suggest adding those in for clarify.

4. The "if" statement at the bottom of template.php looks like it might be better placed inside some Drupal hook, like your royal_olive_preprocess_html() hook for instance.

Apart from those little things everything looks great to me. I'll leave you on "Needs review" for someone else to have a second look. Very nice looking theme, by the way.

5. The royal_olive_branch branch still exists, you can delete it like this:

git branch -D royal_olive_branch
git push origin :royal_olive_branch

The FontAwesome library included as part of this theme uses a license that's not entirely compatible with the GPL licensing requirement for code hosted and distributed using d.o. Take a look at the following for some discussions about this:

• Github: FontAwesome - SIL Open Font License and GPL #1124
• Drupal.org: Allow OFL and GPL2 +FE Font Licenses
• Drupal.org: Include font-awesome into whitelist?

• There are a lot of spots where logic/data retrieval has been added directly to page.tpl.php in here (there are 13 calls to theme_get_setting). I'm pretty sure all of these could get moved to being part of royal_olive_preprocess_page() in template.php to be added to the page structure there (this would let you simplify the code for the display logic on all these things as well).
• If you're going to provide slideshow controls in the theme UI it would make sense to include control of the images there as well.
• I'm not super hot on the hard-coded date formatting that takes place in royal_olive_preprocess_node(), what if I wanted to apply a different format to that?
• How is theme_capture.jpg used? Should it be removed?
• it might be worth noting that this theme adds the jquery.cycle plugin in the README and project page. There are other modules that use this library potentially from other locations, if it's included more than once will this break things? If different versions are included will this break things?
• The version of jquery.cycle that is included says it requires jQuery 1.7.1+ (jquery.cycle.all.js line 8) but Drupal core only ships with 1.4.4. (This still appears to work, but there might be some parts of cycle that don't.)
• Main style.css is kept in the theme root directory instead of in the css directory (with all the other css for the theme), should it be moved to keep things consistent?
• I see the superfish.js and initialize.js files being added to the page via the .info file but it the superfish menus don't trigger on the homepage (only if you're on a page with children and even then only for siblings/children of that page).
• Suckerfish menu appearance is off when using a menu that goes more than two levels deep.

@mraichelson,

1. Removed FontAwesome library from the theme now. Thank You!!

2. theme_get_setting calls in page.tpl.php moved to template.php.

3. No plan to provide slideshow controls for images in the initial release. We may add this feature in the theme UI at a later stage.

4. No plan to make any changes to the Date formatting in the initial release. We may add this control in the theme UI at a later stage.

5. Removed theme_capture.jpg as it was not part of the theme in the first place.

6. Tested again. Works as expected. jquery.cycle plugin gets called through template.php only on the front page. No changes required / made.

7. Have added a note in the README.txt to upgrade jQuery in Drupal core to a latest jQuery version.

8. No. Its the themer's prerogative to place the main style.css where he seems it fit. Works as expected. No changes required / made.

9. Tested again. Works as expected. (Make sure that all menu item having sub-menus have "Show as expanded" ticked). No changes required / made.

10. BTW, its not suckerfish (Superfish jQuery plugin is being used here). Tested again. Works as expected. No changes required / made.

Have moved it back to the previous status - RTBC

superfish.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.

Thanks klausi for the review!

Have removed the 3rd party superfish.js file from the /js directory. Besides, relevant changes made to the .info, template.php and README.txt files.

"Needs Review" is the more appropriate status..

Looks good to me after a manual review.

I'm confused about the user accounts used for the commits here - Binu Varghese you said you implemented the changes, but it appears the commits were made under Vinay Punyamurthy? It seems you are using a non-individual account. All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).

So I would propose that we grant Binu Varghese the git vetted user role, since he seems to do all the work, is that correct Vinay Punyamurthy?

hello klausi,

git vetted user role should go to Vinay Punyamurthy. He is the author of this project. I'm commenting as a maintainer for this project (though i have not committed anything so far).

OK, looks RTBC to me. Assigning to stBorchert as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, Vinay Punyamurthy!

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.