[theme-reviewers] Distilled Review

[Tom Lany]

Thanks for the note.

I noticed that DB query, too, but didn't know how concerned I should be 
about those.  It would probably be best for the author to use the 
get_the_author function instead, as all they appear to be doing here is 
pulling an author name.  If they wanted to pull more information, 
http://codex.wordpress.org/Author_Templates might be a starting place.  
I would agree that this query should be probably be removed, but we 
might want to get feedback on why it was included.

I also noticed that ./lib/pngfix.js has an MIT license.  Is this acceptable?

Tom Lany
http://tomlany.net/


On 6/11/10 1:01 PM, Joseph Scott wrote:
> Terrific, thank you for going through this theme.  A few other items I noticed:
>
> - there's no license info for the theme (that I saw)
> - the Page nav at the top also overlaps the rss feed link.  In cases
> where there is a fixed about of room I usually suggest limiting the
> number of Page entries to display.   Theme authors should be testing
> with the sample data set, but that doesn't seem to always happen
> - the category nav also goes beyond it's original area.  Hoping WP3.0
> and custom menus helps with both these items :-)
> - the theme options are saved as separate WP options.  These options
> aren't particularly big, no reason why they couldn't be saved as
> single WP option
> - there is a direct DB query in page-authors.php, can you see if
> that's something that can be replaced with an existing WP function?
>
> Double check on that DB item and then we can package this up to send
> to the theme author.
>
>
>
> On Thu, Jun 10, 2010 at 8:43 PM, Tom Lany<mail at tomlany.net>  wrote:
>    
>> I reviewed Distilled v. 1.4
>> (http://themes.svn.wordpress.org/distilled/1.4).  Overall, I like the
>> looks.  I just have a couple of concerns:
>>
>> When WP_DEBUG is turned on, I get the following errors in the admin:
>>
>> Top of all admin pages:
>>
>> Notice: Undefined index: page in
>> /home/tomlanyn/public_html/dev/testing/wp-content/themes/distilled/lib/admin/theme-admin.php
>> on line 11
>>
>> When trying to update a pages or change themes, I get this error (in
>> addition to the above error). This error also prevents me from completing
>> the desired action (updating page, changing theme, etc.). I do not have any
>> problems when WP_DEBUG is off.
>>
>> Warning: Cannot modify header information - headers already sent by (output
>> started at
>> /home/tomlanyn/public_html/dev/testing/wp-content/themes/distilled/lib/admin/theme-admin.php:11)
>> in /home/tomlanyn/public_html/dev/testing/wp-includes/pluggable.php on line
>> 868
>>
>> A bunch of errors are present on the distilled options page in the admin
>>
>> Also, this theme uses the depreciated get_the_author_email function on post
>> pages.
>> When the number of links at the very top (navigation bar) exceeds the number
>> of links there are room for, they cover the title below.  Perhaps this is
>> unavoidable, or perhaps the length of this bar could be extended if more
>> links exist.
>> Perhaps the screenshot.png could be sized down to save space (is currently
>> ~250k).
>> Perhaps an error could be added to the 404 page; currently the page appears
>> as a blank template page.  Might an error message be helpful?
>> When multiple pages exist in a post, the author might consider making links
>> to the following pages more apparent.
>> When I view an attachment image on the attachment image page, the image is
>> displayed twice.  I might consider an attachment.php file with the
>> appropriate info.
>>
>> I really like the blockquote styling, and think this theme has great
>> potential, and just needs to have a few code issues polished up.  The colors
>> work well together, and the code is well done as a whole.  The stylesheet is
>> well organized, which should be helpful to new folks.  Keep up the great
>> work!
>>      
>
>
>