Closed GoogleCodeExporter closed 9 years ago
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
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:
+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
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:
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:
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
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
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
assigning ownership...
Original comment by eugene.m...@gmail.com
on 25 Jan 2012 at 6:01
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:
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
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
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
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
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
comments styling punted to issue 132
Original comment by eugene.m...@gmail.com
on 26 Jan 2012 at 2:40
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
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
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
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
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
That got dropped as it was attached to issue 107
Original comment by eugene.m...@gmail.com
on 26 Jan 2012 at 9:19
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
Cool marking as "Verified"
Original comment by eugene.m...@gmail.com
on 26 Jan 2012 at 9:54
Original issue reported on code.google.com by
helgathe...@gmail.com
on 15 Jan 2012 at 5:16Attachments: