WordPress.org

Make WordPress Themes

Opened 12 months ago

Closed 4 months ago

#30863 closed theme (not-approved)

THEME: Nickel – 1.3.2

Reported by: Cohhe Owned by: simonrcodrington
Priority: new theme Keywords: theme-nickel
Cc: support@…

Description

Nickel - 1.0.0

Enhance your story experience, by leveraging the power of Nickel Wordpress Theme, built especially for telling great stories. It is the only free WordPress theme fully compatible with the Aesop Story Engine.

Theme URL - http://cohhe.com/project-view/nickel/
Author URL - https://cohhe.com/

SVN - https://themes.svn.wordpress.org/nickel/1.0.0
ZIP - https://wordpress.org/themes/download/nickel.1.0.0.zip?nostats=1

History:

Ticket Summary Status Resolution Owner
#30863 THEME: Nickel – 1.3.2 closed not-approved simonrcodrington

(this ticket)

#36346 THEME: Nickel – 1.3.3 closed not-approved kevinhaig


https://themes.svn.wordpress.org/nickel/1.0.0/screenshot.png

Attachments (16)

comments_missing_pages.jpg (205.0 KB) - added by simonrcodrington 6 months ago.
Comments missing on 'page.php' template
image_attachment_page_missing.jpg (86.9 KB) - added by simonrcodrington 6 months ago.
Attachment page missing (for viewing single images / media)
menu_nested_styling_issue.jpg (167.9 KB) - added by simonrcodrington 6 months ago.
Nested menu styling after 2nd level. 3rd menu elements inaccessible
missing_tag_name_archive.jpg (150.2 KB) - added by simonrcodrington 6 months ago.
Missing tag name H1 element when viewing the term archive template
post_card_listing_php_error.jpg (134.6 KB) - added by simonrcodrington 6 months ago.
Post listing error with an undefined index. Could have something to do with post formats.
post_format_quote_errors.jpg (181.6 KB) - added by simonrcodrington 6 months ago.
Post format issues. Selecting any format besides 'standard' damages the layout
sticky_post.jpg (71.5 KB) - added by simonrcodrington 6 months ago.
Low contrast issue: When viewing a sticky post, the title when hovered / interacted with goes a similar shade of blue
responsive_nav_issue.jpg (113.6 KB) - added by simonrcodrington 6 months ago.
Responsive issue: When resizing the nav menu and search don't line up (simple styling issue)
posts_navigation_styling.jpg (10.2 KB) - added by simonrcodrington 6 months ago.
Missing Styling: Missing the styling for the ... element in the pagination control. Styling fix.
post_long_title_styling.jpg (160.0 KB) - added by simonrcodrington 6 months ago.
Long post title issue. When using a long title, the text expands out of the card. Suggested fix is to overflow hidden the container.
responsive_menu_toggle.jpg (53.1 KB) - added by simonrcodrington 6 months ago.
Mobile view for toggling the menu. Styling could be adjusted when on small profiles so that the menu and search are useable
undefined_social_media_function.jpg (34.2 KB) - added by simonrcodrington 6 months ago.
As soon as I activate version 1.3 it throws a fatal error.
post_format_link_unclickable.jpg (62.9 KB) - added by simonrcodrington 6 months ago.
When using the link post format you can't click into the single post
post_format_overflow_clickable_issue.jpg (61.6 KB) - added by simonrcodrington 6 months ago.
When viewing the 'Quote' format posts, the meta below (categories etc) are not clickable.
site_title_color_theme_customizer.jpg (86.7 KB) - added by simonrcodrington 6 months ago.
When selecting the title color. It doesn't seem to do anything?
site_title_description_missing.jpg (130.0 KB) - added by simonrcodrington 6 months ago.
When you've uploaded a logo in the theme and then go back and click on the button in the customizer to display the title and description, it doesn't output. The expected functionality should be that it still displays these elements even if a logo has been uploaded.

Download all attachments as: .zip

Change History (48)

#1 @trkr
7 months ago

Hi @Cohhe,

This is just a notice.

Reviewers are now allowed to close the ticket if 3 or more security or prefixing issues are found. https://make.wordpress.org/themes/2016/07/12/meeting-notes-for-2016-july-12/

Please make proper changes before your theme gets reviewed.

Thanks,
Turker.

#2 @themetracbot
7 months ago

  • Summary changed from THEME: Nickel – 1.0.0 to THEME: Nickel – 1.1

Nickel - 1.1

Enhance your story experience, by leveraging the power of Nickel Wordpress Theme, built especially for telling great stories. It is the only free WordPress theme fully compatible with the Aesop Story Engine.

Theme URL - http://cohhe.com/project-view/nickel/
Author URL - https://cohhe.com/

SVN - https://themes.svn.wordpress.org/nickel/1.1
ZIP - https://wordpress.org/themes/download/nickel.1.1.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=nickel/1.0.0&new_path=nickel/1.1

History:

Ticket Summary Status Resolution Owner
#30863 THEME: Nickel – 1.3.2 closed not-approved simonrcodrington

(this ticket)

#36346 THEME: Nickel – 1.3.3 closed not-approved kevinhaig


https://themes.svn.wordpress.org/nickel/1.1/screenshot.png

#3 @themetracbot
7 months ago

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

#4 @simonrcodrington
7 months ago

I'll be reviewing this theme over the next few days. I'll few in touch with my feedback / comments after I've had a chance to test it.

Cheers

#5 @Cohhe
7 months ago

Thank you! Let me know if you need anything from me.

Regards,
Cohhe

#6 @grapplerulrich
7 months ago

  • Owner simonrcodrington deleted

I am sorry this review is taking so long. Sometimes people are unable to carry on the review, this may have happened this time. As a result, I am going to add this to the new queue again as a priority. This may mean it still takes time, but will prevent this ticket being held for so long by a reviewer that isn't able to carry on. Thanks for your patience.

If you are the reviewer and able to do this review, please carry on and request you get added back in Slack #themereview or you can take on another review when you have time again.

#7 @themetracbot
7 months ago

  • Owner set to naudiyal

#8 @grapplerulrich
7 months ago

  • Owner changed from naudiyal to simonrcodrington

@simonrcodrington Please continue the review if you have time. @naudiyal Feel free to help too.

#9 @simonrcodrington
7 months ago

Hey @grapplerulrich thanks for that. Sorry about any inconvenience. I'll continue and post an update soon.

#10 @simonrcodrington
6 months ago

Hi @Cohhe

Thanks for your patience during the review process.

I'm still finalising my feedback, however I will be finished in the next day. I wanted to make sure I test everything thoroughly.

Overall the theme works well, however I've managed to find a few bugs that will need to be addressed before we can approve it. Some of them are simple, while others seem to affect the layout / accessibility of the site.

I'll be in touch soon with my findings.

Cheers


Last edited 6 months ago by simonrcodrington (previous) (diff)

@simonrcodrington
6 months ago

Comments missing on 'page.php' template

@simonrcodrington
6 months ago

Attachment page missing (for viewing single images / media)

@simonrcodrington
6 months ago

Nested menu styling after 2nd level. 3rd menu elements inaccessible

@simonrcodrington
6 months ago

Missing tag name H1 element when viewing the term archive template

@simonrcodrington
6 months ago

Post listing error with an undefined index. Could have something to do with post formats.

@simonrcodrington
6 months ago

Post format issues. Selecting any format besides 'standard' damages the layout

@simonrcodrington
6 months ago

Low contrast issue: When viewing a sticky post, the title when hovered / interacted with goes a similar shade of blue

@simonrcodrington
6 months ago

Responsive issue: When resizing the nav menu and search don't line up (simple styling issue)

@simonrcodrington
6 months ago

Missing Styling: Missing the styling for the ... element in the pagination control. Styling fix.

@simonrcodrington
6 months ago

Long post title issue. When using a long title, the text expands out of the card. Suggested fix is to overflow hidden the container.

@simonrcodrington
6 months ago

Mobile view for toggling the menu. Styling could be adjusted when on small profiles so that the menu and search are useable

#11 @simonrcodrington
6 months ago

Hi again @Cohhe

I've attached a series of screenshots which outline different issues I've found while reviewing the theme.

Several issues I've found are purely styling and should be quick fixes; However some of these affect the accessibility of the theme / or are in regards to supporting key WordPress functionalities.

Here is a listing of my comments. For some of these points I've outlined how to address the issue.

For convenience I've listed them from the most to least severe.

  • When viewing a post with the quote or link formats, the layout is significantly disrupted. Please see image. Formatting for the standard post is fine and works great.
  • Image attachment page missing, no way to view a single media object like an image or document.
  • Comments are missing when enabled on a page by page basis and with comments assigned. Both pages and posts need to support comments if they are enabled. The current page.php doesn't support comment output.
  • Nested menus any deeper than 2 don't work. For example we have a level 1 menu with a sub-menu and inside that another sub-menu. The third nested level is inaccessible.
  • Post archive page. when post with a really long title displayed, it pokes out of it's current card and leaks onto the element below it. This could be stopped with a overflow: hidden on the main article. Please see image above for a visual on how it looks.
  • functions.php around line 135 returns a notice error for undefined offset (as shown in the image displaying your blog). This notice needs to be addressed.
  • Low contrast issue when hovering over sticky post. The title turns a similar shade of blue and becomes diffcult to read. This could be solved with a style change for sticky posts.
  • Post pagination should style the default '...' (as outlined in image)
  • screenshot size 1200x900 are generally required. Your image should work fine but might be small for HIDPI devices.
  • Menu toggle element looks strange on my mobile testing. If you adjusted bottom-header and header-search-container to be full width on smaller profiles the search / mobile toggle section would be more usable.
  • Few validation issues when passing site through: https://validator.w3.org. For example each menu item has an attached ID. When you display the same menu again (e.g inside a widget) it outputs the menu and you get ID duplication (because two menus exist on the same page).
  • Additional prefixing for transformations and transitions should be used (throughout the styling there are some parts where there are un-prefixed values). Vendor prefixing isn't required but highly recommended.
  • On single category page, the title of the category is displayed. On single tags however there is no title displayed. When viewing tag.php this could be displayed by adding the following:

<h1 class="main-page-title"><?php _e('Tag', 'nickel'); echo ': ' . single_tag_title("",false); ?></h1>

  • When resizing downwards, the navigation menu and the search box don't line up. When looking at the styling it seems that the issue is down to the #primary-navigation element taking up too much width. Changing the width slightly smaller such as 69.5% makes them align left and right correctly without collapsing.
  • When sticking a post to the front of the blog, the title of the post when hovered is almost the same blue as the card itself. This would be better if on stickied posts the hover colour changes to something else for readability.

I will continue to go through your theme but those are the biggest elements I've seen so far. Let me know once you've had a change to review my feedback

Cheers

Last edited 6 months ago by simonrcodrington (previous) (diff)

#12 @trkr
6 months ago

  • w3 validation is not a requirement
  • 3rd party scripts and styles should not be prefixed
  • if you want support post formats, each format has distinctive styling or functioning. E.g. Different backgrounds or using some post type in sliders etc.

#13 @Cohhe
6 months ago

Hi @simonrcodrington,

thank you for the review! I have finished everything except the part regarding pingbacks and trackbacks. Can you please clarify what templates are you asking for? I couldn't find info about it in the link you provided.

Regards,
Cohhe

#14 @simonrcodrington
6 months ago

Thanks @Cohhe

Are you able to upload your updated theme here so I can test it again? In regards to the pingbacks I'm going to check that they are 100% required (so I wouldn't worry about that right now)

Kind regards

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


6 months ago

#16 @themetracbot
6 months ago

  • Summary changed from THEME: Nickel – 1.1 to THEME: Nickel – 1.3

Nickel - 1.3

Enhance your story experience, by leveraging the power of Nickel Wordpress Theme, built especially for telling great stories. It is the only free WordPress theme fully compatible with the Aesop Story Engine.

Theme URL - http://cohhe.com/project-view/nickel/
Author URL - https://cohhe.com/

SVN - https://themes.svn.wordpress.org/nickel/1.3
ZIP - https://wordpress.org/themes/download/nickel.1.3.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=nickel/1.1&new_path=nickel/1.3

History:

Ticket Summary Status Resolution Owner
#30863 THEME: Nickel – 1.3.2 closed not-approved simonrcodrington

(this ticket)

#36346 THEME: Nickel – 1.3.3 closed not-approved kevinhaig


https://themes.svn.wordpress.org/nickel/1.3/screenshot.png

#17 @Cohhe
6 months ago

Here it is.

#18 @simonrcodrington
6 months ago

Thanks @Cohhe for the update. I'll check this out today or tomorrow and get back to you.

Cheers

Edit: Just wanted to post an update that I'm still looking through your theme. Thanks for addressing some of the issues I pointed out before (like comments on pages, layout issues on the media attachment pages etc). I should have an update for you shortly.

Last edited 6 months ago by simonrcodrington (previous) (diff)

@simonrcodrington
6 months ago

As soon as I activate version 1.3 it throws a fatal error.

#19 @simonrcodrington
6 months ago

Hey @Cohhe are you able to look at the image above. When activating the theme and navigating to the front-end it throws an error for nickel_get_social_icons. I saw in the version diff you added that call in the header, but it looks like it's not defined or loaded at that point.

let me know when you've fixed this and I'll continue reviewing.

Cheers

#20 @themetracbot
6 months ago

  • Summary changed from THEME: Nickel – 1.3 to THEME: Nickel – 1.3.1

Nickel - 1.3.1

Enhance your story experience, by leveraging the power of Nickel Wordpress Theme, built especially for telling great stories. It is the only free WordPress theme fully compatible with the Aesop Story Engine.

Theme URL - http://cohhe.com/project-view/nickel/
Author URL - https://cohhe.com/

SVN - https://themes.svn.wordpress.org/nickel/1.3.1
ZIP - https://wordpress.org/themes/download/nickel.1.3.1.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=nickel/1.3&new_path=nickel/1.3.1

History:

Ticket Summary Status Resolution Owner
#30863 THEME: Nickel – 1.3.2 closed not-approved simonrcodrington

(this ticket)

#36346 THEME: Nickel – 1.3.3 closed not-approved kevinhaig


https://themes.svn.wordpress.org/nickel/1.3.1/screenshot.png

#21 @Cohhe
6 months ago

Just fixed it.

Regards,
Cohhe

#22 @simonrcodrington
6 months ago

Thanks @Cohhe I'll have a look at this tonight again.

Cheers

Last edited 6 months ago by simonrcodrington (previous) (diff)

@simonrcodrington
6 months ago

When using the link post format you can't click into the single post

@simonrcodrington
6 months ago

When viewing the 'Quote' format posts, the meta below (categories etc) are not clickable.

@simonrcodrington
6 months ago

When selecting the title color. It doesn't seem to do anything?

#23 @simonrcodrington
6 months ago

Hi @Cohhe

I've reviewed your theme. Thanks for updating version 1.3.1. I've imported the test theme data so I could run through how your site handles posts, pages, attachments etc.

Thanks for fixing a majority of the elements I pointed out before.

I've split my feedback into two sections; The top containing errors and the bottom containing recommendations (these are optional but highly recommended)

Errors / Issues

  • When viewing the blog page there seems to be an issue with posts using the 'Link' format. When viewing the single post it all works fine, however in the blog listing, the card that is usually clickable (to go to the single post) isn't clickable. As a test I created a single post, added in a simple link and selected the 'link' format.
  • When viewing a post format page using the 'Quote' format the meta info (with links to tags and categories etc) aren't clickable as they are on normal posts. See attached image above.

Recommendations

  • Nav mennu works with sub-navs now, however if you add nested menus past the third level they are not outputted. For example I had Item 1 -> Item 2 - > Items 3 -> Items 3A. Item 3 displayed but it looks like the custom nav-walker you're using didn't output the next level.
  • When using the customizer. Does the 'Site Title Color' do anything? The background color works fine, but this title setting doesn't seem to do anything?

Other

  • I tried looking for the functionality outlined here on your site, but could't see anything for me to test: https://cohhe.com/project-view/nickel/. Do you have a sample data import that creates multiple homepage templates / layouts? I'm curious.

Let me know when you've had a look through my feedback and we can move forward.

Cheers

Simon

#24 @simonrcodrington
6 months ago

Hey again @Cohhe just wanted to touch base. Let me know when you've made your changes so I can review it again.

Overall everything looked in order so once you've addressed my feedback I'll move this onto the next step.

#25 @themetracbot
6 months ago

  • Summary changed from THEME: Nickel – 1.3.1 to THEME: Nickel – 1.3.2

Nickel - 1.3.2

Enhance your story experience, by leveraging the power of Nickel Wordpress Theme, built especially for telling great stories. It is the only free WordPress theme fully compatible with the Aesop Story Engine.

Theme URL - http://cohhe.com/project-view/nickel/
Author URL - https://cohhe.com/

SVN - https://themes.svn.wordpress.org/nickel/1.3.2
ZIP - https://wordpress.org/themes/download/nickel.1.3.2.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=nickel/1.3.1&new_path=nickel/1.3.2

History:

Ticket Summary Status Resolution Owner
#30863 THEME: Nickel – 1.3.2 closed not-approved simonrcodrington

(this ticket)

#36346 THEME: Nickel – 1.3.3 closed not-approved kevinhaig


https://themes.svn.wordpress.org/nickel/1.3.2/screenshot.png

#26 @Cohhe
6 months ago

Sorry for the delay! Here is the new version.

#27 @simonrcodrington
6 months ago

Hey again @Cohhe thanks for that. I'll review this tomorrow and get back to you.

Cheers

@simonrcodrington
6 months ago

When you've uploaded a logo in the theme and then go back and click on the button in the customizer to display the title and description, it doesn't output. The expected functionality should be that it still displays these elements even if a logo has been uploaded.

#28 @simonrcodrington
6 months ago

Hey @Cohhe

Thanks for making those changes.

I've only spotted one element you might want to think about down the track.

When you've uploaded a logo on the site, the title and description don't display even when selected in the theme customizer (as shown above in the screenshot)

It's a non critical issue so I'm going to mark the theme as approved and move this process along

Kind regards

:)

#29 @simonrcodrington
6 months ago

  • Status changed from reviewing to approved

#30 @greenshady
4 months ago

I'm doing the admin review for this theme.

#31 @greenshady
4 months ago

  • Status changed from approved to reopened

#32 @greenshady
4 months ago

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

Because this theme is well over the 5-issue minimum required to close a ticket, this ticket is being marked as "not-approved". Please take some time to go over all of the issues below and to make sure your theme is up to date with all of the theme review guidelines.

Once you get everything in order, please resubmit your theme for further review.

Missing paging

There's no paging navigation in the following templates:

  • tag.php
  • taxonomy-post_format.php

Google Maps key

What's the purpose of this theme option? Do you have a test key that I can use to test this feature?

Prefixing: Variables

In inc/metaboxes/layouts.php, you have 2 variables in the global namespace: $config and $options. These need to be prefixed or wrapped up within a function.

Prefixing: Classes

In inc/metaboxes/add_meta_boxes.php, you need to prefix the create_meta_boxes class.

Security

In inc/metaboxes/add_meta_boxes.php, you need to sanitize the following before saving it to the database:

$value = $_POST[$option['id']];

Escaping

In inc/metaboxes/add_meta_boxes.php, you must escape the variables used as attributes:

<input type="hidden" name="' . $this->config['id'] . '_noncename" id="' . $this->config['id'] . '_noncename" value="' . wp_create_nonce(plugin_basename(__FILE__)) . '" />

Also, you should just use wp_nonce_field() here to output the entire field.

In inc/metaboxes/templates/layouts.php, you have the same issue of not escaping attributes:

<input type="radio" name="<?php echo $id; ?>" id="<?php echo $id . '-' . $layout; ?>" style="display: none;" value="<?php echo $layout; ?>" <?php checked($selected, $layout); ?>/>
<label for="<?php echo $id . '-' . $layout; ?>"><img src="<?php echo get_template_directory_uri(); ?>/images/layout/layout-<?php echo $layout; ?>.png" alt="" class="<?php if($selected == $layout) echo 'selected'; ?>" /></label>

Additionally, use the selected() WP function to output selected="selected".

Pagination

nickel_paging_nav() must be replaced with the core WP function the_posts_pagination().

Note that this function is defined in both functions.php and inc/template-tags.php.

Missing translation

Make sure to internationalize line 48 of inc/customize.php:

'label'      => 'Google maps key',

Admin scripts/styles

Only load admin scripts/styles on the pages that they are necessary in the admin. Also, styles should be enqueued on the admin_enqueue_styles hook.

From functions.php:

function nickel_admin_css() {
	wp_enqueue_style( 'nickel-admin-css', get_template_directory_uri() . '/css/wp-admin.css' );
}
add_action('admin_head','nickel_admin_css');

And:

add_action( 'admin_enqueue_scripts', 'nickel_admin_scripts' );
function nickel_admin_scripts() {
	wp_register_script('master', get_template_directory_uri() . '/inc/js/admin-master.js', array('jquery'));
	wp_enqueue_script('master');
}

Prefixing: Style handle

Make sure to prefix this style handle in functions.php:

wp_register_style('googleFonts', '//fonts.googleapis.com/css?family=Roboto+Slab:100,300,400,500,600,700|Roboto:100,300,400,500,600,700|Open+Sans:100,300,400,500,600,700|Oswald:100,300,400,500,600,700|Satisfy:100,300,400,500,600,700&subset=latin');
wp_enqueue_style( 'googleFonts');

Also, there's no need for wp_register_style() here. Just use wp_enqueue_style() only in this instance.

Make sure to escape get_permalink() calls in the nickel_prev_next_links() function within functions.php.

And in the nickel_get_menu_category() function.

Prefixing: Constants

The MAGAZINE_LAYOUT constant should be prefixed because it's in the global namespace.

Incorrect use of query_posts()

In nickel_get_menu_category() in functions.php, you should use WP_Query() or get_posts().

Additionally, use wp_reset_postdata() with the latter two. wp_reset_query() should only be used with query_posts().

A word or warning: because you're using this with menu items, this could be an extremely database-intensive bit of code to run, which could result in many, many DB queries. I'd recommend warning users of this danger in your readme.txt file.

Plugin territory: Allowed post tags

Themes should not be overwriting the $allowedposttags global as you have in nickel_allowed_tags(). Leave this to plugins.

Note: See TracTickets for help on using tickets.