Theme Development Checklist

[GoogleCodeExporter]

Made it through the first 2 sections of the code quality part of the checklist: 
php/js/css/html and doctype declaration and have 3 small revisions to submit

1. Theme Development Checklist requires correct content-type meta declaration 
to come before  <title>

2. correct a minor warning from css validator wherein <hr> in default.css had 
same background-color and color.  removed color.

3. not related to the checklist (at least not that i have seen yet).  i think 
the scripts should be enqueued on the wp_enqueue_scripts() hook and not at 
get_header().  also, if you supply the source in wp_enqueue_script() there 
isn't really a need to use wp_register_script as far as i can see.

What version of the product are you using? 
r778

Original issue reported on code.google.com by helgathe...@gmail.com on 15 Jan 2012 at 5:16

Attachments:

• header.php.patch
• default.css.patch
• init.php.patch

[GoogleCodeExporter]

Thanks will push the required for soon and look at the others.

Original comment by eugene.m...@gmail.com on 17 Jan 2012 at 2:45

• Changed state: Started

[GoogleCodeExporter]

3.cont- scripts are required to be enqueued on wp_enqueue_scripts() after all.  
i just hadn't gotten to part in the docu.  

attachments:
changed order of title and content functions in header-extensions.php to go 
along w/ the change in order in header.php

fixed typos in sidebar-extensions.php and theme-options.php

----------------
i've gotten through almost all of the review checklist now and my results are 
in the forum:
http://themeshaper.com/forums/topic/theme-development-checklist

further issues:

4. themes are req'd to use a unique slug for all custom functions, classes, db 
entries, etc.  it is recommended to use the theme-slug (same as textdomain) for 
this purpose.  so i think this means all functions, etc must start with 
thematic_.  most do, but there are several that do not.  

5. comments_class() is required if using a callback for wp_list_comments().  
does not exist in thematic.  i think we might have to use it the same way we're 
using body_class()

6. register_nav_menu and add_theme_support('post-thumbnails') need to be in 
functions.php.  i'd support moving all of init.php back into functions.php

Original comment by helgathe...@gmail.com on 17 Jan 2012 at 4:26

Attachments:

• header-extensions.php.patch
• sidebar-extensions.php.patch
• theme-options.php.patch

[GoogleCodeExporter]

+1 on moving things back to functions.php.

And I like the idea with thematic_init and thematic_child_init actions.

Original comment by invistr...@gmail.com on 17 Jan 2012 at 6:33

[GoogleCodeExporter]

i'm not sure i see the point of the thematic_init and thematic_child_init 
actions.  what is an example of their use?  why isn't thematic_init() added to 
after_setup_theme hook like thematic_child_init is?  

attachment: 
*moved init.php functions back to functions.php.  
*changed from require_once() to locate_template()
*swapped deprecated TEMPLATEPATH for get_template_directory()
*enqueue scripts on wp_enqueue_scripts

this would mean we could delete the init.php file.  

also noticed that some functions say they've been there since 0.9.7.8 even 
though they are new.  what is the version number going to be? should things 
like the theme options be since version 0.9.8 or whatever you're calling this 
release?

Original comment by helgathe...@gmail.com on 17 Jan 2012 at 8:23

Attachments:

• functions.php.patch

[GoogleCodeExporter]

it is required that we now use get_stylesheet_uri() and get_feed_link('rss2') 
instead of get_bloginfo('stylesheet_url') or get_bloginfo('rss2').  patched.

Original comment by helgathe...@gmail.com on 17 Jan 2012 at 8:36

Attachments:

• header-extensions.php.patch

[GoogleCodeExporter]

My understanding is that with thematic_child_init, a child theme can make sure 
to execute code after thematic has loaded. By default, the child theme is 
included before the parent theme (this is why themes like twentyeleven go the 
"! function_exists( 'twentyeleven_setup' )" route for their override 
functionality). If a child theme wants to, say, use some thematic constants or 
other thematic functions, they could use this hook for their code. 

Dunno about the thematic_init though. 

Original comment by invistr...@gmail.com on 18 Jan 2012 at 7:24

[GoogleCodeExporter]

gene, any update on the status of some of these changes needed for theme review?

Original comment by helgathe...@gmail.com on 24 Jan 2012 at 10:02

[GoogleCodeExporter]

In r806 i feel I have tackled the last of the theme unit test issues and made 
the last commits relating to the stylesheets.

I need to review the patches above and commit the noted changes that strengthen 
our compliance to review guidelines. 

The only other major thing here is the new theme options. I had some 
reservations about something but I can't place exactly what it was atm... I 
think I wanted to test and review the options array upgrade process once again.

Original comment by eugene.m...@gmail.com on 25 Jan 2012 at 6:01

[GoogleCodeExporter]

assigning ownership...

Original comment by eugene.m...@gmail.com on 25 Jan 2012 at 6:01

[GoogleCodeExporter]

Found some more minor issues:

- the display:block on the edit link caused it to jump to the next line on the 
comments and trackbacks. Fix: change selector to ".page .edit-link"

- an error notice on attachment pages about undefined property. Fix: 
$post->post_ID should be $post->ID

- an error notice on password protected pages about undefined index. Fix: check 
if password cookie hash has been set

- the default widgets will miss titles on a newly activated theme due to issue 
107. Fix: supply a title to the preset widgets when they get registered. This 
applies only to the thematic three "custom" widgets. It seems that the ability 
to remove the title from a widget will be a thematic feature - the builtin 
wordpress widgets all have a default title if the field is left blank.

Supplied patch addresses all of this.

Original comment by invistr...@gmail.com on 25 Jan 2012 at 7:51

Attachments:

• -126_theme-checklist.diff

[GoogleCodeExporter]

and the list grows.

"It seems that the ability to remove the title from a widget will be a thematic 
feature - the builtin wordpress widgets all have a default title if the field 
is left blank."

they do?  hmmm... ok, apparently they do.  i thought for sure they didn't have 
titles if blank.  well i don't want to be different from WP, but i still think 
it makes sense that if you don't want a title you aren't forced to have one.  

Original comment by helgathe...@gmail.com on 25 Jan 2012 at 8:19

[GoogleCodeExporter]

Yes I also felt it made sense. Especially for the search form.

Some final styling issues:

- Heading 5 and 6 in the content are missing styling. Maybe we can just copy 
over the styles from classic.css? 

- The theme unit test data has headings and lists in the comments. The headings 
have no styling at all and the lists are styled like nested comments with 
border and padding. Don't know how common it is to allow visitors to use these 
tags in the comments and I don't know how important it is for the theme review, 
but but...

- big, ins and q are also missing styling of you want to be pedantic, but I 
question the relevance of that

Original comment by invistr...@gmail.com on 25 Jan 2012 at 8:47

[GoogleCodeExporter]

Good catch on all the above...

The cookie hash notice is one I'd have never have seen. I'm now adding the 
secret keys to my local dev env.

Thanks a ton for reviewing. 

Original comment by eugene.m...@gmail.com on 25 Jan 2012 at 8:58

[GoogleCodeExporter]

comments.php changes in r808. 

I used post_password_required() to kill notices and added @todo

Original comment by eugene.m...@gmail.com on 25 Jan 2012 at 10:39

[GoogleCodeExporter]

I'm ok with no h5 h6 big ins and q. 

The nested lists inside comments are not ideal. I have a patch for it but let's 
punt until the next release. 

The gist... I think we need to move the #comments-list from the wrapping div to 
the ol to have it work efficiently. That's likely gonna break some child 
themes. 

We'll need to be ready... and moved to the new domain where we can better 
control upcoming release announcements before making a change like this. 

Original comment by eugene.m...@gmail.com on 26 Jan 2012 at 2:07

[GoogleCodeExporter]

comments styling punted to issue 132

Original comment by eugene.m...@gmail.com on 26 Jan 2012 at 2:40

[GoogleCodeExporter]

r811 and  r812

I think I addressed everything except the thematic functions<-->init patch.

That issue should be addressed in another ticket at some point. I'll start a 
broader discussion on the devel blog that will encompass that topic.

Please review those commits. I'm a sloppy typist lol. Thanks for all the 
corrections past and future. 

I think the last glaring bits are collecting all of the function namespace 
issues in one list... some have been @todo but not all I'm guessing.

What else am I missing here?

Original comment by eugene.m...@gmail.com on 26 Jan 2012 at 5:30

[GoogleCodeExporter]

Aha... I forgot the comment_cliss() requirement.

Issue 133  

This issue is running long and it's easy to lose important details in tangental 
threads like this one. Other than that I think the other points are all 
addressed. QA report welcome

Original comment by eugene.m...@gmail.com on 26 Jan 2012 at 7:22

• Changed state: Fixed

[GoogleCodeExporter]

will try to look over the changes and see if you caught them all. i'll open a 
new ticket for anything you might have missed.  

Original comment by helgathe...@gmail.com on 26 Jan 2012 at 7:33

[GoogleCodeExporter]

Definitely post new and overflowing issues for new items pertaining to theme 
review.

Please come back here and report to "Verify" the "Fixed" status regarding the 
commits in r811

I'll start marking "Verify" on the issues that get positive QA reports.

Original comment by eugene.m...@gmail.com on 26 Jan 2012 at 7:49

[GoogleCodeExporter]

you got all of mine, w/ the exception of the init patch which we know about and 
if you haven't already, i will open a new ticket for.  i think you got all of 
middlesister's as well... except i see the default widgets in 
thematic_init_presetwidgets have no title?

Original comment by helgathe...@gmail.com on 26 Jan 2012 at 9:04

[GoogleCodeExporter]

That got dropped as it was attached to issue 107

Original comment by eugene.m...@gmail.com on 26 Jan 2012 at 9:19

[GoogleCodeExporter]

ok, then i think this ticket is ok to be considered Fixed. created a new ticket 
for the init issue.  

Original comment by helgathe...@gmail.com on 26 Jan 2012 at 9:41

[GoogleCodeExporter]

Cool marking as  "Verified"

Original comment by eugene.m...@gmail.com on 26 Jan 2012 at 9:54

• Changed state: Verified