WordPress.org

Make WordPress Themes

Opened 3 years ago

Closed 3 years ago

#21616 closed theme (not-approved)

THEME: Creative Block - 1.0.1

Reported by: creativeband Owned by: ryelle
Priority: new theme Keywords: theme-creative-block
Cc: jaeil.han@…

Change History (12)

#1 @ryelle
3 years ago

  • Owner set to ryelle
  • Status changed from new to reviewing

#2 @ryelle
3 years ago

Sorry for the review delay, I thought I'd be able to get to it before the holidays. I should have a review for you in the next day or so.

#3 follow-up: @ryelle
3 years ago

REQUIRED

Hardcoding styles and scripts in header.php is not allowed. Add your stylesheet, jquery, and comment-reply script in functions.php, on wp_enequeue_script. See _s_scripts function.

In header.php, remove the call to html5.js, or add the file.

Use get_search_form() to get the proper code for search forms, rather than hard-coding the <form>. If you need to change the markup, you can do so with the searchform.php template.

Add escaping for:

  • header.php, line 38: get_option('cb_header_logo');
  • footer.php, lines 5, 8, 9, 13-16: all echo get_option

Text domain must match the theme slug, creative-block. I've found twentyfourteen in comments.php, search.php, archive.php. I also see theme_domain in functions.php, line 88.

Make sure that all your user-facing text is translatable. I see a few strings that are not run through translation functions in the customizer setup.

All functions need to be prefixed with a unqiue, theme-specific slug. You can use creative_block_, for example. You'd want to change your my_masonry function to:

function creative_block_masonry(){
	wp_enqueue_script( 'masonry' );
}
add_action( 'wp_enqueue_scripts', 'creative_block_masonry' );

This should be done for all functions.

add_theme_support and register_nav_menus should be done inside a function, see _s_setup, which hooks into after_setup_theme.

readme.txt is empty. This should contain licenses of any resources included such as fonts or images, and a changelog (which can just say "intial release" for now). I also usually add some short documentation, but that isn't required.

Pages should have comments. The user can disable comments if they want to, but you should support them.


RECOMENDED

You can merge the JS in footer.php, and maybe move it to a separate JS file. Put everything into one jQuery(document).ready(function($){}); block, for example.

Check if the user has set social links before printing the images. Currently, if not, you just display icons that don't go anywhere.

Similarly, check for phone & fax before printing the T & F. I just have a mystery "T. F." in my footer: https://cloudup.com/c0JmuSR6dy4

Since you don't seem to use any different markup, you can merge tag.php and category.php into archive.php, and add another check to change the archive-title.

Change the "SNS Link" section heading to something more user-friendly, like "Social Networks".

For wp_nav_menu, you don't need to list out the $defaults each time, just pass in what you want changed. For example, your main navigation can be:

$defaults = array(
	'theme_location'  => 'main_nav',
	'container'       => false,
	'container_id'    => 'main-navigation'
);
wp_nav_menu( $defaults );

Overall a very nicely designed theme! I especially like the animation on single posts' featured images. Let me know if you have questions on any of my feedback.

#4 in reply to: ↑ 3 @creativeband
3 years ago

Thank you very much, ryelle!

I'm trying to modify my theme, now.
But I have a question about "Add escaping for: ".
Actually I don't know how to fix that.

Could you send a sample code or web article link about 'add escaping' ?

Thanks.

#5 @ryelle
3 years ago

Sure - here's a pretty thorough guide about working with data in WordPress, and towards the end it talks about escaping and has some examples.

There's also this page in the codex which links to all the relevant core WordPress functions - when I say "escaping," I'm talking about the "output sanitization" section here.

#6 follow-up: @ryelle
3 years ago

Hi there! We usually close reviews after 7 days of inactivity - can you comment back in the next day if you're still working on this theme? Just a quick "still going" kind of note is great.

Thanks!

#7 @themetracbot
3 years ago

  • Summary changed from THEME: Creative Block - 1.0 to THEME: Creative Block - 1.0.1

#8 in reply to: ↑ 6 @creativeband
3 years ago

Replying to ryelle:

Hi there! We usually close reviews after 7 days of inactivity - can you comment back in the next day if you're still working on this theme? Just a quick "still going" kind of note is great.

Thanks!

Sorry, ryelle.
Cause I have to study about data validation and sanitisation, update of modified version is too late.

Thanks.

#9 @ryelle
3 years ago

Cause I have to study about data validation and sanitisation, update of modified version is too late.

Not too late! I'll review your update & get back to you.

#10 @ryelle
3 years ago

Here you go, sorry it took me a few days.


REQUIRED

I got a PHP notice when I activated your theme: "Warning: call_user_func_array() expects parameter 1 to be a valid callback, function '_s_setup' not found or invalid function name in /srv/www/multisite/wp-includes/plugin.php on line 496" -- You need to update the hook to use your setup function, on functions.php Line 36.

The search form still needs work:

  • Submitting the non-mobile form doesn't work, since the input name is not "s". Can you use the same form for all browsers (mobile and not)?
  • Hardcoding the action as "/index.php" will break on subfolder installs of WordPress - like if someone has their WP install at sitename.com/blog/. Searching here will send the search term to sitename.com/index.php, which might not be WordPress, might not even exist. See the default form here for the code to use.

Escaping:

From header.php:

<a href="<?php echo esc_attr(esc_url( home_url( '/' ))); ?>"><img src="<?php echo get_option('cb_header_logo'); ?>" alt="<?php bloginfo( 'name' ); ?>" /></a>

This is a good start, but there are still some issues. You only need one escape function per output, but you definitely need one for each user-submitted value. So, you can remove the esc_attr from home_url, and you're correct that bloginfo can stay "unescaped" (because it's escaped in core). The get_option call can be set by the user, so you'll want to make sure to esc_url that -- just to make sure it's formatted correctly as a URL, and doesn't contain anything unexpected.

This is how it should look:

<a href="<?php echo esc_url( home_url( '/' ) ); ?>"><img src="<?php echo esc_url( get_option('cb_header_logo') ); ?>" alt="<?php bloginfo( 'name' ); ?>" /></a>

In footer.php, Line 4:

<img src="<?php echo esc_attr(esc_url(get_option('cb_footer_logo', 'blogname'))); ?>" alt="<?php bloginfo('name'); ?>"/>

Passing in the default 'blogname' means that a broken image shows up if there's no image set- it's trying to load <img src="http://blogname" alt="Theme Review">. Here you're doing duplicate escaping again, too.

You don't need to escape get_template_directory_uri.

Untranslated text:

  • 'Main Navigation' in functions.php Line 9
  • 'Page Right Sidebar', functions.php Line 85.
  • Your Archive titles (archive.php)
  • 'Edit this entry.' single.php Line 28, page-fullwidth.php Line 27, page.php Line 23
  • "Permanent Link to " index.php Line 21 (this one can be tricky, with the link in text, see how the continue reading link is done using `sprintf` here)
  • You've used creative-block as the domain in 404.php, which is technically different than creative_block.

The arguments for add_theme_support( 'custom-background', $args ); in functions.php Line 23 redundant or incorrect- only use the things you've changed from the default.

Since you use setup_postdata in sidebar.php, you should make sure to use wp_reset_postdata after the loop (for example, after the endforeach) to reset the site back to the original queried post. more info here.

RECOMMENDED

footer.php Line 24: 2014 &copy; <?php echo get_option('cb_company', 'blogname'); ?>. Falls back to "2014 © blogname". You could change the default to get_bloginfo('name'), and use the date function to generate the current year: date('Y').


You've made great progress, and even though this is long, your main issues are translation and escaping- which are hard to get right when you're learning :)

If you're worried about taking longer than 7 days for the next update, just drop a note here that you're still working. As long as you're in contact, this ticket will stay open.

#11 @ryelle
3 years ago

And once again I forgot an important step -- You need to include information about the licensing for assets you use. In readme.txt, list out the sources and licenses for all your images (even if you made them yourself). For example, in one of my themes that uses Modernizr, I have this in my readme:

== Licenses ==

Adirondack is released under GNU General Public License v2.0
License URI: http://www.gnu.org/licenses/gpl-2.0.html

Adirondack bundles the following third-party resources:

Modernizr 2.8.3 (Custom Build)
License: MIT
Source: http://modernizr.com/

#12 @ryelle
3 years ago

  • Resolution set to not-approved
  • Status changed from reviewing to closed

Closing due to inactivity. If you want to continue, please resubmit your theme, and you'll be put back in the review queue.

Note: See TracTickets for help on using tickets.