WordPress.org

Make WordPress Themes

Opened 3 years ago

Closed 2 years ago

#23430 closed theme (not-approved)

THEME: Apricot Blog – 1.1.6

Reported by: nish@… Owned by: alex27
Priority: new theme Keywords: theme-apricot-blog
Cc: nish@…, karmatosed

Description

Attachments (3)

apricot-blog.zip (538.6 KB) - added by nish@… 3 years ago.
Theme Apricot 1.1
apricot-blog.2.zip (539.5 KB) - added by nish@… 3 years ago.
Apricot 1.1.1
apricot-blog.3.zip (540.6 KB) - added by nish@… 3 years ago.

Download all attachments as: .zip

Change History (43)

#1 @themetracbot
3 years ago

  • Owner set to vlad.olaru
  • Status changed from new to reviewing

#2 @vlad.olaru
3 years ago

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

Hi,

Nice one :)

Now what things I have found so far that need your attention:

  • The search widget in the right sidebar may benefit from some styling, just in case someone want's to use that one instead of the one in the header area;
  • You care not displaying comments on pages, when they are enabled;
  • With the debug mode activated, on the blog page I get quite a few warnings for null given on line 81 in index.php (it's because the posts don't have a featured image and the $featured is null). You should always work with debug enabled and use the Theme Unit Test demo data to test things
  • In the same Theme Unit Test there is a post about image alignment. When an image is bigger than the container it overflows the container and gets cut. I would suggest making sure that an image is never bigger than the container (max-width comes to mind)
  • When posts are paginated, I would suggest showing the pagination also on the archives, not just in single post view (also maybe some styling to that post/page pagination would be nice to keep the design consistent and top notch :) )
  • On single post, in the comments area, I get a notice "Notice: Undefined variable: image " in functions.php on line 272 (I guess because I don't have an avatar and the email of the admin doesn't have a gravatar account)
  • In another post from Theme Unit Test, there is an edge case, where a post doesn't have content. On archives, you are not displaying the title, making it impossible to reach the single post view. On single the title doesn't get shown.
  • Please make sure that all your PHP files have a proper header so we can keep some part of the WordPress standards. The default themes are a great inspiration. The file header should be somewhere along these lines:
/**
 * Some description of the template/file
 *
 * Maybe additional details
 *
 * @package Apricot
 * @since Patch 1.0
 */

That's it for now :)

Best regards,
Vlad
PixelGrade

#3 @vlad.olaru
3 years ago

Oh and one more. Please make the theme screenshot an actual site screenshot.

#4 @nish@…
3 years ago

How do I submit the changes?

#5 @vlad.olaru
3 years ago

You need to resubmit it with an higher version than the current one (this is why it is recommended that when you submit a theme for review to use sub zero version numbers).

#6 @jcastaneda
3 years ago

  • Resolution not-approved deleted
  • Status changed from closed to reopened

Vlad, please give the theme author time to upload a revised version. As common courtesy we allow up to 7 days. Reopening for this reason.

@nish@…
3 years ago

Theme Apricot 1.1

#7 @nish@…
3 years ago

When posts are paginated, I would suggest showing the pagination also on the archives, not just in single post view (also maybe some styling to that post/page pagination would be nice to keep the design consistent and top notch :) )

The archive pages have page numbers. I believe you are referring to year-archive, month-archive, etc ...

Other than this, I have made all corrections. Attached is version 1.1.

Thank you for taking the time to review. This is my first theme.

#8 @vlad.olaru
3 years ago

Hi,

Regarding the pagination, I was referring to individual posts that have page breaks in them (<!--nextpage-->).

Vlad

@nish@…
3 years ago

Apricot 1.1.1

#9 @nish@…
3 years ago

I have added styling to paginated posts. See attached theme.

#10 @vlad.olaru
3 years ago

Ok. Good to see that.

Some other things that I have noticed:

  • When dealing with the gallery post format, you are correctly extracting the first gallery and creating a slider our of it, but at the same time you are hiding through CSS all the galleries in the content, even if there are more than one. That is not a good practice. Either you don't extract the first gallery, on single post view, and display everything, or you extract that first one and eliminate it from the content.

Maybe something along these lines (I couldn't find a tutorial somewhere)

function sometheme_strip_first_content_gallery( $content ) {
	if ( 'gallery' == get_post_format() ) {
		$regex   = '/\[gallery.*]/';
		$content = preg_replace( $regex, '', $content, 1 );
	}

	return $content;
}

add_filter( 'the_content', 'sometheme_strip_first_content_gallery' );
  • Regarding the menu areas, I would use a more standard menu ID, like 'primary' so people won't loose the menu location asignation when they switch themes (and freak out). The default themes use this id also:
'primary' => __( 'Primary Menu',      'twentyfifteen' )

That's about it.

Vlad
PixelGrade

#11 @nish@…
3 years ago

Good points!

Made the changes.

#12 @nish@…
3 years ago

Any news? How / when does the theme get approved?

#13 @vlad.olaru
3 years ago

  • Status changed from reopened to approved

Hi,

Sorry for the delay but I returning from a vacation. We are good to go here.

#14 @nish@…
3 years ago

So, how does it go from here? When / how is the theme released?

#15 @vlad.olaru
3 years ago

Now a more senior reviewer needs to put it live.

This ticket was mentioned in Slack in #themereview by karmatosed. View the logs.


2 years ago

#17 @karmatosed
2 years ago

  • Status changed from approved to reopened

I am not seeing a new version uploaded just zips. We can't accepts zips as to be reviewed. Please upload your latest version for a check here.

@vlad.olaru please then do a review on that.

One major thing you need to look at from a glance is the screenshot. This should be of the theme.

#18 @themetracbot
2 years ago

  • Summary changed from THEME: Apricot Blog - 1.0 to THEME: Apricot Blog – 1.1.2

#19 @karmatosed
2 years ago

  • Cc karmatosed added

#20 @nish@…
2 years ago

Any update?

This ticket was mentioned in Slack in #themereview by nishant. View the logs.


2 years ago

#22 @grapplerulrich
2 years ago

  • Owner changed from vlad.olaru to jancbeck
  • Status changed from reopened to reviewing

Thank you @jancbeck for completing this review.

#23 @jancbeck
2 years ago

Hello @nish@

I've reviewed the theme and these are the things I noted:

I recommend you remove the empty lines in your header.php before you declare a DOCTYPE. This could potentially lead to problems in some cases. Simply use this to avoid it:

<?php
/**
 * The template for displaying Header
 *
 * @package WordPress
 * @subpackage Apricot
 * @since Apricot 1.0
 */
?><!DOCTYPE html>

Things like post view tracking a la line 300 in functions.php are considered plugin territory. I strongly suggest you move the widget to a plugin of their own and make user opt-in to tracking that information.

Please sanitize all your Customizer API input. Functions like

function apricot_sanitize_logo( $input ) {
	return $input;
}
function apricot_sanitize_basic( $input ) {
	return $input;
}

are not valid sanitization functions. Read franks post about the topic: https://make.wordpress.org/themes/2015/06/02/a-guide-to-writing-secure-themes-part-3-sanitization/

You can remove

/* Mother Script */
wp_enqueue_script( 'jquery' );

since you already declared jQuery as a dependency of apricot-script that you enqueue a few lines later.

You often use $_SERVER['HTTP_HOST'] without checking wether the key is present in $_SERVER. When running WordPress over CLI this will show a notice. Please use isset to avoid this. Also, why are you frequently testing for $_SERVER['HTTP_HOST']=='localhost' || $_SERVER['HTTP_HOST']=='ncrafts.net'?

Throughout the theme there is code that seems to be some kind of contact form except there is only JavaScript and CSS.
functions.php Line 195:

add_action('wp_ajax_apricot_form_submit', 'apricot_form_submit');
add_action('wp_ajax_nopriv_apricot_form_submit', 'apricot_form_submit');
function apricot_form_submit()
{
    if ( empty($_POST['name']) || empty($_POST['email']) || empty($_POST['comment']) )
...

What is your intention here? Please note that contact forms are considered plugin territory and this code should be moved there.

Please explain/change the points I noted above to continue with the review process.

Last edited 2 years ago by jancbeck (previous) (diff)

#24 @nish@…
2 years ago

I had a contact module, but removed that. I have now removed the associated JS and CSS as well. I have also removed the page view tracking code, and references to localhost (it was for the theme preview site).

I have also fixed the empty lines issue, and removed the redundant jquery enqueue.

Thanks for the review!

#25 @themetracbot
2 years ago

  • Summary changed from THEME: Apricot Blog – 1.1.2 to THEME: Apricot Blog – 1.1.3

#26 @jancbeck
2 years ago

Thanks for the updates @nish@. I've continued the review and noticed a few more things:

Required changes:

  1. All untrusted data should be escaped before output. I noticed that especially in the author widgets you are directly outputting user meta data without properly escaping it:


$output .= "<div class='site_author_image $color'>$author_link<div class='site_author_tag'>$tag</div><img alt='$name' src='$image'/></div>";
		$output .= "<div class='site_author_content'>";
		$output .= "<div class='site_author_bio'>$bio</div>";

This is very dangerous and can lead to serious security implications. Please make use of the esc_attr and esc_html and read their codex pages. https://codex.wordpress.org/Function_Reference/esc_attr https://codex.wordpress.org/Function_Reference/esc_html

  1. In functions.php line 795 you are using esc_sql as a sanitize function. Please use sanitize_text_field instead.
  2. When not selecting a custom font, the webfont.js script throws an error Uncaught Error: No fonts to load!. Themes must not produce any JS or PHP errors.

Recommendations:

  • Some of the changes in the Customizer are not reflected in the preview. Especially the two layout options would not work when I tested them. However when I did save and close the Customizer and load the page, the changes were there. Maybe you could have a look so these settings would be previewed correctly.
  • As I was experimenting with the two-column-layout I noticed some problems with content that is wider than their containers e.h. images and videos. See this screenshot: http://i.imgur.com/ayZLabQ.png
  • When the header is sticky there is very little space for the site title: http://i.imgur.com/7qZT9Ae.png However there is plenty of space to the right of the navigation. Perhaps you find a little more elegant solution so the users site titles won't get chopped off.

Other than that I have nothing to complain. Good work, @nish@!

#27 @themetracbot
2 years ago

  • Summary changed from THEME: Apricot Blog – 1.1.3 to THEME: Apricot Blog – 1.1.4

#28 @nish@…
2 years ago

I've made all changes, except the issue related to some changes not appearing live in the theme preview. I have no idea how to get that working.

#29 @jancbeck
2 years ago

Thanks for the updates nish@…

You missed item number two though:

In functions.php line 795 you are using esc_sql as a sanitize function. Please use sanitize_text_field instead.

Using sanitize_text_field is a save bet for most of the fields that involve a text field.

Also, please make correct use of esc_attr and esc_html. An example of correct usage would be this:

echo '<a href="' . esc_attr( $author_link) . '>' . esc_html( $author_name ) . '</a>';

Please do not use display:none to hide the screen-reader-text class as in line 1803 of style.css. The text will become inaccessible to screenreaders. See this article for more information about how to handle the screenreader class: https://make.wordpress.org/accessibility/2015/02/09/hiding-text-for-screen-readers-with-wordpress-core/

Please make sure there are no hardcoded <script> and <style> tags in your header.php. The correct way to pass PHP values to CSS is the wp_add_inline_style function: https://codex.wordpress.org/Function_Reference/wp_add_inline_style
The correct way to pass PHP values to JavaScript is either through the localize_script function or via the API of the Customizer. Have a look at this codex page: https://codex.wordpress.org/Theme_Customization_API#Part_2:_Generating_Live_CSS

The labels of the Customizer controls should be translatable.

#30 @jancbeck
2 years ago

Quick update: wp_head() should be called as the last thing before </head>

#31 @themetracbot
2 years ago

  • Summary changed from THEME: Apricot Blog – 1.1.4 to THEME: Apricot Blog – 1.1.5

#32 @nish@…
2 years ago

I've made all changes

#33 @jancbeck
2 years ago

Thank you for the changes. It looks all good now except for the usage of esc_html and esc_attr. You use esc_html for what is output between HTML tags and esc_attr for what is output as an attribute of an HTML element.

line 826 of functions.php:

$output .= "<div class='site_author_image $color'>$author_link<div class='site_author_tag'>$tag</div><img alt='$name' src='$image'/></div>";

You can see $color, $image and $name are attributes. $tag is output between HTML tags.
Therefore this

$name = esc_html($name);
$color = esc_html($color);
$user = esc_html($user);

should be

$tag = esc_html($tag);
$image = esc_attr($tag);
$name = esc_attr($name);
$color = esc_attr($color);
$user = esc_attr($user);

The code above is just an example. Please go through all of your variables that are output publicly and correctly sanitize them. All social variables are currently using esc_html where they should be using esc_attr.

#34 @themetracbot
2 years ago

  • Summary changed from THEME: Apricot Blog – 1.1.5 to THEME: Apricot Blog – 1.1.6

#35 @nish@…
2 years ago

Should be okay now.

#36 @jancbeck
2 years ago

  • Status changed from reviewing to approved

Thank you. The review is now completed.

#37 @alex27
2 years ago

  • Status changed from approved to reopened

#38 @alex27
2 years ago

Hi there!

Unfortunately I need to reopen this ticket, as there are still issues that need addressing:

  • Change 'menu' to 'theme-location' in wp_nav_menu call in header.php
  • All theme options need to be escaped on output. See: https://codex.wordpress.org/Validating_Sanitizing_and_Escaping_User_Data#Escaping:_Securing_Output
  • Theme URI (if used) needs to link to a page dedicated to the theme, for example with more info / setup instructions.
  • Please provide license and credit info for all assets bundled with the the theme (including images in the screenshot).
  • Mixed text domains found ‘apricot’ and ‘apricot theme’;
  • add_action( 'wp_enqueue_scripts', 'apricot_scripts_and_styles', 999 );
    add_action( 'widgets_init', 'apricot_register_sidebars' );
    

should not be included inside apricot_initialize;

  • load_theme_textdomain() and add_editor_style calls should be placed inside apricot_theme_support(). Ultimately the apricot_initialize function is not needed at all.
  • Unprefixed function found: colourBrightness();

#39 @alex27
2 years ago

  • Owner changed from jancbeck to alex27
  • Status changed from reopened to reviewing

#40 @alex27
2 years ago

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

Closing due to inactivity. When you have an update ready, just upload new version of your theme, and someone will pick up the review.

Note: See TracTickets for help on using tickets.