Elegant Blue

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

Hi, I found a few issues and I have a few comments as well:

1. What is the attribution and source of tms-0.4.x.js in the js folder? I don't see any license info in that file so you may have to remove it if it does not conform to the standards of drupal licensing.
2. Why do you have jquery-1.7.min.js included? I am guessing this could cause some issues for users. If this is necessary, the proper way would be to suggest users install JQuery Update.
3. You have quite a bit of logic in your template files (page--front.tpl.php for example). I would suggest anything that has theme_get_setting should go into template.php as a preprocess function perhaps and then create a variable that can be dropped into page--front.tpl.php or page.tpl.php.
4. The front page hard coded blocks "Three Column Blocks" seems a bit untidy but I get that you are trying to provide some quick content out of the box. When I deleted the Text and title in the theme settings from one of the blocks, it broke the layout. In addition, the images were broken. See attached screen capture.
5. The $block = module_invoke('search', 'block_view', 'form') seems a bit untidy. I would at least provide a theme setting to turn that off if a user does not want it.
6. You should call Google fonts without http so it will work on secure and non secure sites.
   @import url(http://fonts.googleapis.com/css?family=Vollkorn); should probably be:
   @import url(//fonts.googleapis.com/css?family=Vollkorn);

   ... but you may need to do this as a preprocess function in template.php if that does not work. You have a bunch of these called throughout so they would need fixing. see #1921294: Google Fonts cause unsecure warnings when using SSL for reference.

7. In page--front.tpl.php, you have:
   if (module_exists('i18n')) etc.. with curly braces, it would probably be better to use the alternative php syntax here but probably even better would be to take the logic out and put it into template.php somehow as a preprocess function. See "Alternate control statement syntax for templates" on http://drupal.org/coding-standards -- You are actually already using that type of syntax elsewhere so that's good. Also see Alternative syntax for control structures for reference.
8. I added some sidebar blocks but they did not show up on the home page as I am guessing you don't render those in page--front.tpl.php. At the very least, perhaps add a note about this to the README, this could confuse some users otherwise.

Finally it would have been nice if this theme were responsive but that's just my 2 cents, not really a thing you have to do. I'm just not sure about the viability of a non-responsive theme these days, especially a contrib theme.

Hello Ankit,

Some Quick Comments :

1. Better to use l() instead of direct href.

   Example : page--front.tpl.php :

   <?php if ($facebook): ?><li class="fb"> <a href="<?php print $facebook; ?>" target="_blank"> </a> </li> <?php endif; ?>
   

   Lots of use of href and instead of l(), also using l() you will able to open the link in new window or tab.

2. ; missing after endif

   Example : page--front.tpl.php : line number - 88

        <div class="home <?php if ($is_front): print "act"; endif?>">
        should be
        <div class="home <?php if ($is_front): print "act"; endif;?>">
        

3. Use theme_image to populate the images

   Example : page--front.tpl.php : line number - 112

        <img src="<?php print base_path() . drupal_get_path('theme', 'elegant_blue') . '/images/slide-image-2.jpg'; ?>" alt="" />
        

4. Suggestion : Provide capabilities to add/render required information on images.
   Example : Would be great if you provide user to add the alt and title tag for an image, they are key elements for the SEO.
5. UI Issues :
   Example : Drop Down Menu is not working on Access Denied Pages.
   Example : Cosmetic CSS changes for Cross Browser support needs some work. Example : On Tabs Padding-top is not consistent.

Thanks,
Ujval Shah
Drupal Consultant

Your i18n check should check for i18n_menu not i18n as the i18n module can be present without the i18n_menu module being activated which results in a fatal error.

Hi Anks - You've renamed the file (and function) for "tms-0.4.x.js" to "slide.js" in your repo but the code is the same. You still have not stated what the attribution or license of that is. Drupal is pretty strict about including 3rd party libraries and scripts. I saw various references to that script in a Google search but no clear source and license.

Hi,

I had review the theme manually and found some bugs. Please see the following.

1. I had create a basic page with plain text. When i open it, It's breadcrumb is something wrong. Refer screenshots(breadcrumb.png).
2. In Firefox, Some UI of search button is disturbed. See screenshot(search.button.png).
3. In main menu, All links are clickable on it's hover(I mean all black part is clickable on hover). But in case of home link, Only home images is clickable. Don't know exactly that it is an issue or something else but please take a look on it.

Code Review:

In info file, It would be better if you add some comments at the starting of any stylesheet, scripts, or regions. Please see the Info file here.

Other than that, it's a nice looking theme !

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. slider.js: do not use jQuery(document).ready(), use Drupal.behaviors instead. See http://drupal.org/node/756722
2. elegant_blue_preprocess_page(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as welcome text description I get a nasty javascript popup. You need to sanitize user provided text before printing, please read http://drupal.org/node/28984 again.

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

Forgot attachment.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

• I think it is more appropriate to apply filter_xss_admin() sanitization in elegant_blue_preprocess_page() where you prepare the variables instead of doing it in the templates. A template should be dumb - it should not know which variables need to be sanitized and which don't. It should only print them at the right place to HTML.

But otherwise looks RTBC to me.

Assigning to mlncn as he might have time to take a final look at this.

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

Thanks for your contribution, Anks!

I updated your account to let you 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 get 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.