WordPress.org

Theme Review Team

Recent Updates Page 2 Toggle Comment Threads | Keyboard Shortcuts

  • Tammie 9:34 pm on March 3, 2015 Permalink |  

    Theme review team weekly notes

    The logs are here:
    https://wordpress.slack.com/archives/themereview/p1425405735000714

    We talked about queues.
    We are going to post two tickets to meta. One will be to ask for amendments to the assignment button workflow to include for existing reviewers tickets unresponded to for 7 days. We are also going to look at getting a clean up script that closes tickets where an author hasn’t responded for 7 days.

    We also talked about images and licenses.
    With regards to model releases, for now we are going to not consider this as our review and go by the licence provided. So business as it was.

     
    • Ulrich 10:20 pm on March 3, 2015 Permalink | Log in to Reply

      I hate to be the nay sayer now but this is not the best solution. It is not right for a ticket to have to wait 7 days for no activity so to be reassigned. We want people to communicate with each other. When anyone comments in the ticket or mentions the ticket in Slack then the modified date gets updated. I do not see the difference to this system to the tickets being set to “new” so that it gets re-added to the assignment button workflow.

      Sorry for not mentioning it in the Slack discussion. It was not clear for me it the discussion.

      I fear that it will not be so easy to find out when the author has not replied in 7 days. It is not that important.

      • Tammie 12:38 pm on March 6, 2015 Permalink | Log in to Reply

        We have to have a time period. You can’t expect everyone to be participating in daily conversations. The difference is that tickets don’t sit without anyone assigned to them. Marking as new does that, which isn’t good.

        As I said in the meeting, I personally am not keen on anything that reassigns like this from a psychological view. This however, is a better way than simply flagging back to new.

    • Jasmine Jazz 2:37 am on March 5, 2015 Permalink | Log in to Reply

      Hi I don’t know a lot but do need someone to look at my theme and help me out please.

  • Tammie 9:22 pm on March 2, 2015 Permalink |  

    Agenda for this week’s meeting

    We’ve got a meeting at 18:00 UTC on Tuesday (tomorrow) in Slack #themereview.

    A suggested agenda:

    • Lets talk abandoned and missing in action tickets. How can we handle them better? What mechanism can we have to make this more robust?

    Any other subject people would like to talk about?

     
    • Tammie 9:22 pm on March 2, 2015 Permalink | Log in to Reply

    • Patryk Kachel 7:08 am on March 3, 2015 Permalink | Log in to Reply

      Maybe we can reassign abandoned tickets via button? First move them to “abandoned” queue and then assign via “request button”. Maybe we can even move tickets automatically to “abandoned queue” when there is no response for 7 day.

    • nitkr 8:50 am on March 3, 2015 Permalink | Log in to Reply

      +1 for Urlrich’s suggestion for abandoned ticket, but now it seems a reviewer cannot re-assign it.

      If a theme author is aware about their ticket queue priority(which will be high) for re-assignment, it will surely bring down the amount of re-assign notifications an admin receives.

      I also think, rather CC’ing an admin about re-assign and closed-newer-version-uploaded tag, if they are also made available for reviewers it could be handy.

      • Ulrich 10:53 am on March 3, 2015 Permalink | Log in to Reply

        The problem is that we are unable to set the themes to status new. @otto42 Do you think it would be possible to get an option added to the UI to be abe change the status from reviewing to new?

        • Tammie 10:56 am on March 3, 2015 Permalink | Log in to Reply

          I would be against that. It’s a psychological step back for any review to be then put as new again. We also have the knock on effect onto a few reports then. Lets not have that as an option.

      • Tammie 10:57 am on March 3, 2015 Permalink | Log in to Reply

        A reviewer shouldn’t be able to re-assign. Finding a way to do this without having re-assigning on the reviewer side is essential.

        • nitkr 11:44 am on March 3, 2015 Permalink | Log in to Reply

          I agree with you, as I could see admins are getting notifications for re-assignment, I guess re-assign option could ease an admins work(a bit).

          I did see those re-assign options for re-opened tickets of navsingh, so that made me ask.

    • Maria Antonietta Perna 10:24 am on March 3, 2015 Permalink | Log in to Reply

      Last week I plucked up my courage and decided to click the Request a theme to review button. I’m not an expert and I only had my first WP.org blogging theme approved less than a couple of weeks ago (it’s not live yet). However, I read about the presence of mentors for new reviewers and this convinced me to take the plunge. I pressed the button and I got assigned a feature-rich magazine theme with quite a few theme options and other stuff. At the same time, I don’t know who the mentor for this ticket is or if I can talk to him/her about my concerns. On the other hand, the options on the ticket are quite restrictive: approve or don’t approve and I don’t feel that I can make this decision by myself at this very early stage. I hope today’s meeting will be of help.

      • Ulrich 10:50 am on March 3, 2015 Permalink | Log in to Reply

        Great, We are working on improving the mentor process. When reviewing the theme you should leave it on status reviewing so that the ticket stays open and the theme author can upload updates. If you have any questions you can ask them in the Slack #themereview channel. There is always someone there who can help. Ping me on Slack if you have any more questions. :)

    • Maria Antonietta Perna 11:09 am on March 3, 2015 Permalink | Log in to Reply

      Thanks, Ulrich!

  • Tammie 12:19 am on February 27, 2015 Permalink |  

    This week’s (a little late) meeting notes

    We met on Tuesday for our weekly meeting. This is every week at 18:00 UTC. You can find this week’s archive here:
    https://wordpress.slack.com/archives/themereview/p1424800773003737

    Notes:

    • @obenland came into talk about the theme directory which is being worked on over the next few days. It’s going to be exciting to see when done.
    • @davidakennedy came into talk about the accessibility pattern library. He’ll be doing a write up on this and we’ll post it here as a link too.
    • @greenshady whilst wasn’t there brought to our attention how important checking the history of a theme you’re reviewing is.
    • @ryelle brought up the important talking point of design feedback. We had a good discussion hearing a lot of opinions and reviewing what has and hasn’t worked of us in the past. We all agreed it was important and a long path, but we needed to start on that long path. @melchoyce will be starting us off with filling out the design section of our recommendations: https://make.wordpress.org/themes/handbook/review/recommended/design/. After that, we can look at next steps and keep this as a topic we come back to and keep important.

    Contribution days with theme review:

    This weekend will be a contribution day at Brighton and Hove, UK. There will be a theme review section there. http://www.meetup.com/WordUp-Brighton/events/218684194/
    Are you planning on holding a theme review session at a contribution day or a session on it’s own? Let us know and we can tell everyone about it.

     
  • Tammie 5:17 pm on February 23, 2015 Permalink |  

    Agenda for this week’s meeting

    We’re going to have a meeting this week at 18:00 UTC on Tuesday in #themereview on Slack.

    We have one topic so far:

    • Accessibility snippet library

    Add your topic below and we’ll try and get to it, however if we have to many we’re going to have to spread them over a few weeks.

     
    • Kelly Dwan 8:55 pm on February 23, 2015 Permalink | Log in to Reply

      • Expectations for commenting on tickets you aren’t “reviewer” on. From this twitter thread, I think we should be more explicit about how this is encouraged – for example, add a note somewhere on the handbook suggesting partial reviews for people with less time, or who aren’t comfortable doing a full code review.
      • Design standards for the review handbook’s recommended design page (could easily be a meeting on its own, i’m sure)
      • Tammie 9:00 pm on February 23, 2015 Permalink | Log in to Reply

        Good points – lets add those to the agenda and get some time on them. Thanks for bringing them up.

    • Justin Tadlock 11:15 pm on February 23, 2015 Permalink | Log in to Reply

      Point of discussion: Reviewers need to be checking the previous ticket on theme update reviews for notes from the previous reviewer or admin. There have been several times where I’ve left notes for the theme author and next reviewer only to find out that the next reviewer didn’t check this.

    • Justin Tadlock 5:16 pm on February 24, 2015 Permalink | Log in to Reply

      I’ll have to miss out on the meeting today. Have fun!

  • Jose Castaneda 10:01 pm on February 17, 2015 Permalink |  

    Handbook Examples: Functionality and Features 

    Last week we began with the code section of the handbook. This week let’s get some momentum and tackle: Core functionality and features.

    Functionality and Features examples:

    Example:
    There is no need to use
    add_theme_support( 'menus' );
    because register_nav_menu() does it behind the scenes.

    Example:
    Use core defined functions when possible
    So rather than using your own functions:

    function my_custom_cats(){
    	$categories = get_the_category();
    	$catOut = '';
    	$separator = ', ';
    	$catOutput = '';
    	if($categories){
    		foreach($categories as $category) {
    			$catOutput .= '<a href="'.get_category_link( $category->term_id ).'" title="' . esc_attr( sprintf( __( "View all posts in %s", 'text-domain' ), $category->name ) ) . '">'.$category->cat_name.'</a>'.$separator;
    		}
    		$catOut = 'Categories: '.trim($catOutput, $separator);
    	}
    	return $catOut;
    }
    

    Use a core function like the_category()

     
    • Justin Tadlock 10:35 pm on February 23, 2015 Permalink | Log in to Reply

      Use get_template_directory() rather than TEMPLATEPATH to return the template path.

      require_once( trailingslashit( get_template_directory() ) . 'inc/example.php' );

      Use get_stylesheet_directory() rather than STYLESHEETPATH to return the stylesheet path.

      require_once( trailingslashit( get_stylesheet_directory() ) . 'inc/example.php' );

      This example should come with an additional warning to check if the file exists first if not used in a child theme.

      Include comments_template().

      comments_template();

      Should be called in at least all singular views.

  • Jose Castaneda 7:30 pm on February 17, 2015 Permalink |  

    Chat Notes 

    Slack Archives: https://wordpress.slack.com/archives/themereview/p1424196039002725

    Topics:

    Queue

    • some improvement in last 5/6 months
    • button assigning

    Frameworks

    • can take long time
    • ‘sos’ checkbox ( looking at you #meta team )
    • education ( tuts and stuff )

    Child theme plugins

    • add support for plugin but not styling
    • not allowed
     
  • Jose Castaneda 8:38 pm on February 16, 2015 Permalink |  

    Chat Agenda

    Topic: Frameworks

    If you have any other topics you would like to discuss please post them here.

    Looking forward to seeing you there! Same time and place: Slack #themereview 1800UTC

     
  • Justin Tadlock 9:19 pm on February 10, 2015 Permalink |  

    Handbook Examples: Code Section 

    Beginning today, we will be discussing examples for the handbook requirements. Ultimately, these examples will end up on the Explanations and Examples page in the handbook.

    Each week, we’ll focus on various sections. This week’s section is the Code section. What we need from everyone is to provide examples in the below comments. The admins will round up the examples and add them to the handbook.

    Code section examples

    I’ll get us started with a couple of examples just to get the ball rolling.

    Required: Have a valid DOCTYPE declaration and include language_attributes.

    <!DOCTYPE html>
    <html <?php language_attributes(); ?>>

    Required: No shortcodes are allowed.

    The use of the add_shortcode() function is not allowed in themes as shown below:

    add_shortcode( 'shortcode_name', 'shortcode_callback_func' );
     
  • Justin Tadlock 8:57 pm on February 10, 2015 Permalink |  

    Custom CSS boxes in themes 

    Recently, we’ve had a big discussion over whether themes could allow users to enter CSS into arbitrary text boxes (custom CSS). There was some confusion over what was allowed and, if allowed, what would be the proper method of handling it. After much debate, we’ve come to the following conclusion:

    • It’s preferred that theme authors leave this feature to plugins.
    • However, it is allowed if handled safely.
    • The edit_theme_options capability is required (like all theme options).
    • The wp_filter_nohtml_kses(), wp_strip_all_tags(), or equivalent function must be used to sanitize the data before it’s saved into the database.

    Why leave to plugins?

    There are cases where it makes sense for custom CSS to be applied globally and to be applied on a per-theme basis. Generally-speaking, that’s a decision that’s best left up to the user, which they can decide based on the various plugins available for handling this.

    Validating CSS. Our guidelines are not going to require that theme authors validate the CSS. Our guidelines are only concerned with making sure the data is safe. That means that users could add in arbitrary code that’s not valid CSS. This means it’d be on you, the theme author, to handle the inevitable support requests.

    You are, of course, welcome to use a library such as CSSTidy to tidy the CSS up, but that’s a large library and probably best left in plugins. Just realize that the more complex your theme code is the harder it is to review and the longer it’ll take us.

    Edit: Another good reason to leave this out of themes is multisite. There are many instances where a site owner wouldn’t want everyone with a blog on their site to have this sort of CSS editing capability. Adding this option will most likely mean that a good number of multisite installs will never use your theme.

    Edit #2: To add to the multisite scenario, it might be worth considering using the edit_themes capability instead of edit_theme_options.

    How to do it correctly?

    I’m going to share an example of handling this correctly based on the comments from our recent discussion. This is only going to focus on the Customizer because it’s our recommended way to handle theme options.

    The following is example code to start from:

    <?php 
    
    add_action( 'customize_register', 'prefix_theme_customize_register' );
    
    function prefix_theme_customize_register( $wp_customize ) {
    
    	$wp_customize->add_section(
    		'prefix_section_1',
    		array(
    			'title'       => __( 'Section 1', 'prefix' ),
    			'description' => __( 'Section 1 description.', 'prefix' ),
    			'priority'    => 30
    		)
    	);
    
    	$wp_customize->add_setting(
    		'example_css',
    		array(
    			'default'              => '',
    			'capability'           => 'edit_theme_options',
    			'sanitize_callback'    => 'wp_filter_nohtml_kses',
    			'sanitize_js_callback' => 'wp_filter_nohtml_kses'
    		)
    	);
    
    	$wp_customize->add_control(
    		'prefix_example_css_control',
    		array(
    			'label'    => __( 'Custom CSS', 'prefix' ),
    			'section'  => 'prefix_section_1',
    			'settings' => 'example_css',
    			'type'     => 'textarea'
    		)
    	);
    }
     
    • Ryan Hellyer 9:03 pm on February 10, 2015 Permalink | Log in to Reply

      For anyone looking for an example of correctly handling CSS, I recommend checking out the Safe CSS plugin from Automattic …
      https://wordpress.org/plugins/safecss/

      It handles both CSS validation and sanitization. As Justin points out above, this does result in a rather large code base.

      • Ryan Hellyer 9:04 pm on February 10, 2015 Permalink | Log in to Reply

        I am referring here to how the data is handled BTW, not the UI which is best handled the way Justin described above.

        • Zane Matthew 12:47 am on February 28, 2015 Permalink | Log in to Reply

          So your saying the best way to store CSS is a custom post type? I’ve never thought of that approach, and would love to here what others have to say.

          • Justin Tadlock 12:22 am on March 1, 2015 Permalink | Log in to Reply

            I’d probably use a CPT myself if building this sort of plugin. It makes pretty good sense to me.

            Ryan is specifically talking about how the data is sanitized/validated from what I understand. How the data is stored is actually a separate matter.

    • Zulfikar Nore 12:48 am on February 11, 2015 Permalink | Log in to Reply

      Thanks for the example code Justin.

      Question: What would be the best method to escape the output?

      Zulf

      • Justin Tadlock 4:49 pm on February 11, 2015 Permalink | Log in to Reply

        Just hooking it to `wp_head()`? I’m not sure you’d want to escape it. If you’re allowing users to enter arbitrary code, you’ve given them a lot of rope to hang themselves with. Most of the escaping functions would probably break some CSS.

        I’ll give it some thought.

    • webriti 5:24 am on February 11, 2015 Permalink | Log in to Reply

      What’s the perfect implementation in case of option framework

      • Justin Tadlock 4:39 pm on February 11, 2015 Permalink | Log in to Reply

        The Customizer has been around for two years or so. It’s time to move on to using the core-supported standard for theme options. That’s my official recommendation.

        It’d really depend on which option framework you’re using, but you’d have to use the required capability check and one of the functions mentioned above to sanitize.

        • KProvance 12:08 am on February 20, 2015 Permalink | Log in to Reply

          Why? It’s sucks, and I’m not just saying that out of bias. It’s kludgy and incredibly difficult to code for. I get that the core devs think it’s the bee’s knees…and in some ways (live preview) it is, but as far as dev friendly, and easier for the user to understand. No. And I’ve been designing software for decades, so I know just a bit of where I speak.

          • Justin Tadlock 3:01 am on February 20, 2015 Permalink | Log in to Reply

            Standardization is probably the best answer as to why. I imagine TRT will one day require this for all theme settings. That’s the general direction we’re heading.

            If you think it’s not good enough yet, get involved. Create tickets on Trac. Post wireframes. Discussion on the TRT blog isn’t going to help. Involvement with core might.

    • nikeo 8:21 am on February 11, 2015 Permalink | Log in to Reply

      Thanks for sharing this @greenshady

    • nvartolomei 8:55 am on February 11, 2015 Permalink | Log in to Reply

      Why not leave this for children themes? If user knows what css is and what to do with it i think he can manage to understand what children theme is and how to customize it.

      Why do you need this in customizer?

      • Justin Tadlock 4:35 pm on February 11, 2015 Permalink | Log in to Reply

        I agree with that. Plus, a user could install a plugin if they really didn’t want to do a child theme.

        I also think there’s a case to be made for users who are just “getting their feet wet” and trying out CSS for the first time.

    • Stanko Metodiev 11:53 am on February 11, 2015 Permalink | Log in to Reply

      I’m glad that you’ve decided to not to restrict this option to the plugin territory. I my opinion if the user is administrator he/she can break his/her site in several other ways, so he/she should be able to add Custom CSS. Thanks for the code example, too! :)

    • Dovy Paukstys 10:25 pm on February 19, 2015 Permalink | Log in to Reply

      Disappointing I did all this work to modify my framework when you hadn’t reached a decision. Please don’t post on framework issue trackers before you decide your requirements. Reverting to what we had before so we can pass theme-check again.

      • Justin Tadlock 3:13 am on February 20, 2015 Permalink | Log in to Reply

        You were contacted after TRT had reached a decision. We just happened to later change that decision because I personally brought it back up for discussion on behalf of you and others with similar theme options. You were also given an opportunity to discuss this at the time here on the previous post.

        If you don’t want TRT to post on your issue tracker, you should not have it open to the public or allow people from our team to post on it. We will always take any steps we feel necessary to help theme authors and users, particularly when there’s a question of security involved.

  • Jose Castaneda 7:38 pm on February 10, 2015 Permalink |  

    Weekly Chat notes 

    Slack link: https://wordpress.slack.com/archives/themereview/p1423591238002102

    Topic: quality

    • example comments post ( @jcastaneda )
    • handbook examples

    Sub-topic: child themes

    • Thinking ahead:
      • Parent theme updated in 2 years
      • when to suggest fork

    Re: Custom CSS

    • theme territory, security is key
    • use proper capability check and sanitize
    • CSSTidy for validation
    • SECURITY
     
c
compose new post
j
next post/next comment
k
previous post/previous comment
r
reply
e
edit
o
show/hide comments
t
go to top
l
go to login
h
show/hide help
shift + esc
cancel