[theme-reviewers] help with review
michael at mfields.org
Sat Dec 24 11:43:55 UTC 2011
> also, it is echoing a string without escaping it.
> Author is arguing his code is ok and secure regardless.
> what should I do?
If the value is contained in an attribute, it should always be escaped if it contains a dynamic value. Translated strings are dynamic once translated. This can definitely cause issues with translated strings that include a single quote and the attributes value is enclosed in single quotes. The following is the best way to handle a situation such as this:
<input name="s" type="text" id="s" value="<?php esc_attr_e( 'Search', WEBFISH_THEME_NAME . '-theme' ); ?>"/>
Twenty Eleven provides a similar feature to what the theme author is going for here using the placeholder attribute. You may want to suggest that the author tries something like this instead:
<input type="text" class="field" name="s" id="s" placeholder="<?php esc_attr_e( 'Search', 'twentyeleven' ); ?>" />
My opinion is that the value of the search input be reserved for the search query. The term "Search" in the case is either a label or an action to be taken and should be reserved for the submit button, a label element or a placeholder attribute. Using it as the value of search input is bad form as id decreases usability of the form.
> also, there is no margin below paragraphs, (readability test) - shouldn't that be required?
I believe so. Paragraphs should be easily distinguished from one another.
More information about the theme-reviewers