[theme-reviewers] help with review

[Michael Fields]

Hi Paul,

> 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.

Best,
-Mike