Hiya

welcome,

Please take a moment to create a README.txt that follows the guidelines for in-project documentation.

Also add your .gitignore to the .gitignore so it'll be ignored by git.

Please take a moment to make your project page follow tips for a great project page.

The result of automated review looks good.

You can also get a review bonus and we will come back to your application sooner.

regards

I've just had a bit of an in depth look at the hiya theme. These are my comments:

• Add in a screenshot. It makes an aesthetic difference and allows the user to have a preview on the appearances screen.
• The little calendar image for the date only shows on page and article content types (see attached pic)
• I wonder whether you should remove the javascript from your template.php and instead attach it within javascript.js
• Referencing the above file. I'd recommend having your js file named hiya.js In case it conflicts with other js files and also for easy debugging!
• On a node edit page. I think there should be some text next to the arrow for input formats. Also, when the fieldset is expanded, the arrow moves to the bottom and still faces the same way. Standard fieldset practice is for expanded fieldset arrows to remain in the same place and instead point down rather than right.
• Is there any reason for repeated code? Perhaps you forgot to remove Home and replace with $title?
  

          <?php if ($title): ?>
            <h1 class="title grid_9">
              <a href="<?php print $front_page; ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>
            </h1>
          <?php else: /* Use h1 when the content title is empty */ ?>
            <h1 class="title grid_9">
              <a href="<?php print $front_page; ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>
            </h1>
  

Changing the issue title back ;)

Regarding the calender image, personally I think that consistency in site design is quite important. With some content types displaying the calendar image and some not it just looks a bit disjointed.

Your argument could potentially be that blogs and articles are time sensitive things that should go on a calendar but what about an 'events' content type? Would that not also require a calendar.

Not marking as needs work as this is just an aesthetic change that I'll leave to the theme dev to decide on, just giving some food for thought!

This looks pretty good. I installed it on the test site I'm using for my own theme development and it works reasonably well.

There are a few text elements that could use some attention. Check em tags, h3 - h6, lists, definition lists, tables, and blockquotes. Paragraphs have no margin around them. If you're going to use a CSS reset it's important to make sure all these things get added back in. The Style Guide module can help you to identify things like this when you're working on your theme. Some attention to link hovers would also be helpful. Very few link types have over states, and with some of them (like on the node headers), the change is so subtle it's barely noticeable.

I glanced briefly over the CSS code ... why is there a full CSS reset in a separate file (reset.css), and then another reset in style.css around line 49-117? That's very strange. And, that reset.css is being called last, which means it's overriding and reseting much of what's in the other three files. Why? That's why the text styles aren't appearing properly. They're set in text.css but then cleared by reset.css.

It would be helpful to put some comments in your CSS to mark different sections and keep the code organized.

I installed the theme on my local machine. Played around and reviewed code. For me it looks good.

I found:

• I guess theme logo is missing (see attachment)
• When logged in as admin, I could not click into the search input field (using Chrome)
• Personally I would put somewhere mention that it uses Google Fonts API, maybe in README.tt
• Personally I would put a margin-bottom:20px to the footer, just that the footer does not touch the browserwindow

These changes look good to me. I have some final comments but to me these are minor "TODO" items, not release blockers. I'm therefore marking this as reviewed.

1. README.txt is excessively delimited - a single line of ---- under a heading is fine for a one-page file!

2. There's extra whitespace in the template files that doesn't add to readability (node--article.tpl.php, multiple blank lines in between blocks, but not the same amount - sometimes 2, sometimes 1, sometimes 3). It's only a few extra bytes but it would be nice to be consistent.

3. It wasn't obvious to me after installing it how to switch between the two layouts. It would be very nice if this was a theme setting or exposed to CTools in some way so a Panels/Context based site could set it, or if it hooked a class instead of a variable for the same reason.

4. The theme overrides traditional form elements like author, which is common, but until I poked through template.php I didn't realize what it was doing specifically for articles and blogs (when I tested it, I didn't have these types enabled until I started reading the code). It would be nice to call these behaviors out in the README.txt file for admins who are new to the theme.

5. The JS file uses a document ready() rather than a Behavior. This isn't forbidden - Behaviors are preferred, but there are use cases where document ready calls are correct. This particular one is a grey area because you're almost never going to see the search form loaded via AJAX, and certainly not as you use it in your theme. I recommend documenting this decision in the comment block above the function (or implementing it as a Behavior, where there would be no concern).

6. Because your CSS file spans many pages, as a convenience for themers new to your project it would be a good idea to add comment blocks above major groups.

In the end, I believe these are nice-to-haves that should be worked on over time, but not necessarily as blockers to this becoming a full project. As a starting point for general theming work I think this is a good basis, it is usable as-is out of the box, looks good, and does not have any significant problems that I was able to find.

Hi varshith,

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, at your discretion.

Now that you've experienced the full review process, please consider reviewing other projects that are still awaiting review. Anyone can help with reviews, following the guidelines.