Good day!

I've just submitted a theme I've been working on for about a year and a half now. This theme is different from any other theme on Drupal, as it follows (to a point) the Canadian Treasury Board Common Look and Feel 2.0 Web Standard, and nobody else here has made a project as far as I've seen.

I know, two commits (wow!) doesn't show a very long history. I've been in the web developing scene for some years now, and I'd like to reach out to the community and offer what I can.

I am willing, able and available to commit time and efforts into supporting this project, Drupal and the community.

For your consideration,

Kyle J. Wight

Project page: http://drupal.org/sandbox/kjwight/1114450
Websites actually utilizing this theme: http://www.vetsarmy.com / http://www.nfinitek.ca

Comments

cleaver’s picture

Hi Kyle,

FYI, I'm only seeing the info file in the git repo...

kjwight’s picture

Neato. Git is something new for me. I thought I'd added everything and committed it all :S

Checking out my repo to double-check.

Kyle

kjwight’s picture

There we go, I got my issue all straightened out.

Should all be intact, just checked and did a pull.

sreynen’s picture

Status: Active » Needs review
sreynen’s picture

Issue tags: +PAReview: Theme

Tagging for easier finding.

mindewen’s picture

Issue tags: +pdx-code-review
sarah_p’s picture

Component: new project application » theme
mindewen’s picture

Assigned: Unassigned » mindewen
mindewen’s picture

Hi, Kyle,

After spending an hour or so installing the theme, doing some minor testing with node creation and popular contrib modules, and looking over the actual code, I have a few notes.

First, some concerns:

  • Using the logo as a background image is somewhat unconventional and could cause confusion for site admins. For one thing, people usually expect the logo to perform a specific function, and many people will probably get confused when that functionality is missing. Also, I did not see much documentation as to how I should use this feature, and had to read through the code detective-style to figure it out.

    You may want to look at using theme settings to achieve the same effect, which would not only free up the logo for its normal use but also provide an understandable interface for users to make the customization. The documentation page for Creating advanced theme settings has some good info, and Lullabot has a blog post that steps through how to add customizable header images via theme settings. In that post the author links to a theme, Singular, by Development Seed that achieves basically the thing you're going for, as far as I can tell. Looking through their code could give you some good pointers.

  • The pages do not validate (using W3Cs' validator) due to an extra closing div. This should be a pretty easy fix. :) To prevent this in my own code I usually add comments after my closing divs, to mark which opening they belong to, like so:
    <div class="content">
    	Content
    </div><!-- .content -->
    

  • There are some extra CSS files and code snippets, such as splash.css and lines 3-8 in node.tpl.php (which, by the way, threw out an error when I loaded a test node in a vanilla D6 install). They appear to be leftover customizations from previous projects. The cleaner a theme, the more useful for themers (or this one, at least). :)

Tips/suggestions:

  • As per the Theme Coding Conventions, it's best to wrap PHP inside HTML, instead of the other way around.
  • Standardizing your code indentation will help other themers read your code more easily.
  • Site titles conventionally link to the site home page, so to prevent confusion for visitors you may want to restore that capability.
  • The theme should declare a character encoding in a meta tag.
  • If you decide to put in a unique theme setting, such as a customizable header background image, provide instructions in the README.txt.

Additionally, we may want someone more knowledgeable in best PHP security practices to take a glance through, too.

In general I think this is a good start to a theme and--assuming there are no security vulnerabilities that I have overlooked and providing Kyle is responsive to the concerns I raised--I feel it can be promoted to full project.

mindewen’s picture

Assigned: mindewen » Unassigned
cleaver’s picture

Status: Needs review » Needs work

Just setting the status...

kjwight’s picture

Thank you for the review and insight. I'll be working on this over the weekend to try to get it up to snuff. I'll be reviewing this a little more closely before I resubmit, but, as we all know as coders, it's always best to get a second set of eyes on the code to show you where you went wrong or how you can make it better.

Overall, I don't think it was a review that ended in tragedy, and there wasn't that much picked at, so I think this should be a fairly simple update, and I have a few changes in mind already.

Again, thanks, and I'll be resubmitting soonish.

tim.plunkett’s picture

Status: Needs work » Closed (won't fix)

Closing due to inactivity, feel free to re-open if this was a mistake.