WordPress.org

Make WordPress Themes

Opened 2 years ago

Closed 23 months ago

#27137 closed theme (closed-newer-version-uploaded)

THEME: Jot – 1.0.1

Reported by: kevinhaig Owned by: iamjolly
Priority: new theme Keywords: theme-jot
Cc: kevin@…

Description

Jot - 1.0.0

Jot is a full featured responsive theme designed for mobile devices, but looks cool on full screens too :)

Theme URL - http://kevinsspace.ca/demo4/
Author URL - http://kevinsspace.ca/

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

History:

Ticket Summary Status Resolution Owner
#27137 THEME: Jot – 1.0.1 closed closed-newer-version-uploaded iamjolly

(this ticket)

#29166 THEME: Jot – 1.0.2 closed live iamjolly


https://themes.svn.wordpress.org/jot/1.0.0/screenshot.jpg

Change History (19)

#1 @karmatosed
2 years ago

This is just a note, the screenshot should be as close to what happens when you default load the theme. A mocked up image or single image will prevent the theme from being passed for review. Please resolve this before the review can be completed.

#2 follow-up: @kevinhaig
2 years ago

Hi @karmatosed :)

I'm a little confused.

This screenshot shows actual screen loads in both a tablet and a mobile device.

So it is showing actual theme material.

#3 follow-up: @karmatosed
2 years ago

It should show one or other not both. A screenshot shouldn't be a mocked up image like this.

#4 in reply to: ↑ 2 @kevinhaig
2 years ago

Replying to kevinhaig:

Hi @karmatosed :)

I'm a little confused.

This screenshot shows actual screen loads in both a tablet and a mobile device.

So it is showing actual theme material.

I looked at a bunch of themes.....Yep you are correct :) Thanks for the Heads Up, I will fix it in the next update.

I don't know why but I thought being able to show how the theme displays in both a tablet and on a mobile device would be a good thing for users to see but I guess requirements won't allow that :(

#5 in reply to: ↑ 3 @kevinhaig
2 years ago

Replying to karmatosed:

It should show one or other not both. A screenshot shouldn't be a mocked up image like this.

Yep you are correct :)

#6 @themetracbot
2 years ago

  • Summary changed from THEME: Jot – 1.0.0 to THEME: Jot – 1.0.1

Jot - 1.0.1

Jot is a full featured responsive theme designed for mobile devices, but looks cool on full screens too :)

Theme URL - http://kevinsspace.ca/demo4/
Author URL - http://kevinsspace.ca/

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

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

History:

Ticket Summary Status Resolution Owner
#27137 THEME: Jot – 1.0.1 closed closed-newer-version-uploaded iamjolly

(this ticket)

#29166 THEME: Jot – 1.0.2 closed live iamjolly


https://themes.svn.wordpress.org/jot/1.0.1/screenshot.jpg

#7 @kevinhaig
2 years ago

The screenshot is no longer a mock up, but an actual screenshot of the theme.

#8 @themetracbot
23 months ago

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

#9 @kevinhaig
23 months ago

Are you planning to do this review or can I ask for a new reviewer?

#10 @iamjolly
23 months ago

Nearly done - will complete this today. Since this is my first review, Sakin may need to check it over. I'll make sure we get it approved or back to you quickly!

#11 follow-up: @iamjolly
23 months ago

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

Theme Review of Jot

Hi @kevinhaig!

I have been reviewing your theme, and below are my findings from the review along with recommendations and notes. Please let me know if you have any questions or additional info to provide.

Findings/Results

Your theme is not yet ready for release into the theme repository. There are a few changes, listed in the Required section below, which need to be made. Additionally, please see the Recommendations and Notes sections for additional items you may want to make.

Required

Code

I see the following issues that need correcting:

You haven't "Sanitized everything", so please look at https://codex.wordpress.org/Data_Validation.
Some examples are in file jot-custom-functions.php as follows:

  • In jot_header, you need to sanitize all instances of <?php bloginfo('description'); ?> and <?php bloginfo('name'); ?>
  • In jot_archive_header, the following aren't sanitized:
    • echo get_avatar( get_the_author_meta('user_email') , 70 ) ;
    • echo '<p>'.get_the_author_meta('description').'</p>';

There may be more issues like this to correct throughout all of your theme's files, so I encourage you to check everything to ensure it's sanitized properly.

I'm seeing that there are some JavaScript functions that aren't prefixed in the file kha-customize-mcat.js. They include (but there may be others):

  • function addCategory( catID, controlID ) {
  • function removeCategory( catID, controlID ) {

Licensing:

Please list the license(s) for:

  • Trim Excerpt PHP Script
  • Responsive Video (and if fitvids.js is separate from the Intrinsic Ratio Method ALA article modifications)

Screenshot

  • The Screenshot should be of the actual theme as it appears with default options, not a logo or mockup.

Your screenshot is _technically_ how this theme can appear _if_ someone happened to load up a featured image with two mockups of itself in small/mobile and tablet formats. However, if we're being _technical_, this screenshot contains mockups which aren't allowed. Let folks see the responsiveness of the design via the demo site.

Recommendations

Your theme lists full-width as one of it's tags. I can only see that a full-width layout is possible with the portfolio template, but not with other "standard" pages. You may want to note that for users in your readme file.

Notes

It's really clear to me that you are an experienced developer with a good knowledge of how to build a responsive theme for WordPress. It would be great to have you extend your theme(s) to also be "accessibility-ready" to be more inclusive for a wider range of audiences. You wouldn't have too much work to do with this theme to acheive that, so I encourage you to consider accessibility!

Do let me know if you have questions or further info to provide for this review.

Thanks,
Robert Jolly

Last edited 23 months ago by iamjolly (previous) (diff)

#12 follow-up: @jcastaneda
23 months ago

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

Hi @iamjolly if you could, please allow the theme author some time to submit a revised version. We generally allow up to seven days for them to submit.

#13 @jcastaneda
23 months ago

  • Status changed from reopened to reviewing

also, please fix your formatting, it keeps making a lot of the content bold. :)

Always funny when that happens - at least I think it is.

#14 in reply to: ↑ 12 @iamjolly
23 months ago

Replying to jcastaneda:

Hi @iamjolly if you could, please allow the theme author some time to submit a revised version. We generally allow up to seven days for them to submit.

Ahh, yes. I didn't really know if the status should change but definitely want to keep it open for the fixes from the author. Thank you!

And formatting... ugh. Sorry! I'm clearly learning my way around here. :)

Last edited 23 months ago by iamjolly (previous) (diff)

#15 in reply to: ↑ 11 @kevinhaig
23 months ago

Replying to iamjolly:

Thanks for taking the time to Review my theme. I am a veteran reviewer myself and I know the effort it takes. Please do not take any comments below negatively. I truly want to find the correct way to do things :)

I see the following issues that need correcting:

You haven't "Sanitized everything", so please look at https://codex.wordpress.org/Data_Validation.
Some examples are in file jot-custom-functions.php as follows:

  • In jot_header, you need to sanitize all instances of <?php bloginfo('description'); ?> and <?php bloginfo('name'); ?>
  • In jot_archive_header, the following aren't sanitized:
    • echo get_avatar( get_the_author_meta('user_email') , 70 ) ;
    • echo '<p>'.get_the_author_meta('description').'</p>';

There may be more issues like this to correct throughout all of your theme's files, so I encourage you to check everything to ensure it's sanitized properly.

Comments on data validation.

Just for your info, sanitization is usually the term for validating the data before saving to the database. Escaping is the term for validating data before it is displayed in a page html.

I do not feel that WordPress functions need to be escaped before output. I usually trust that WordPress will handle the security of their own functions. In all my time on slack and reviewing themes the only exception to this that I have seen is home_url().

Option data should be properly escaped before output to page html, and I believe I have done that for all user options used in the theme.

Also, note that many options are switches that are used in conditional statement. In this context the options do not need to be escaped because they are not outputting html to a page.

I believe in escaping only what is required, in order to reduce the function calls when a page is generated, helping to improve page load speed.

I'm seeing that there are some JavaScript functions that aren't prefixed in the file kha-customize-mcat.js. They include (but there may be others):

  • function addCategory( catID, controlID ) {
  • function removeCategory( catID, controlID ) {

These functions do not need to be prefixed because they are wrapped in a jQuery call already.

Licensing:

Please list the license(s) for:

  • Trim Excerpt PHP Script

Trim Excerpt is a simple script available from a bloggers website. I have copied the script verbatum in the fuction comments. There is no specific licensing available. I listed it in the readme.txt file because I want to ensure the author recieves proper credit for his efforts :)

  • Responsive Video (and if fitvids.js is separate from the Intrinsic Ratio Method ALA article modifications)

I will add additional detail to the declaration in the next update.

Screenshot

  • The Screenshot should be of the actual theme as it appears with default options, not a logo or mockup.

Your screenshot is _technically_ how this theme can appear _if_ someone happened to load up a featured image with two mockups of itself in small/mobile and tablet formats. However, if we're being _technical_, this screenshot contains mockups which aren't allowed. Let folks see the responsiveness of the design via the demo site.

The screenshot is not a mockup, it comes from a slide (not a feature image), and was generated by the theme. Also note that the requirement does not mean that it must be a screen shot from the .org demo, at least that is not how it is interpreted by many reviewers. It must be a screenshot that can be generated by the theme. That is what my screenshot is. Granted it does show the theme in both a large media format and a mobile media format which I believe is good for the users to see.

My first screenshot was a mockup for sure, because it was not generated by the theme. But the one I am using now is fine I think. Please check out the screenshots of the following themes which offer comparable screenshots; Customizer, Responsive, Evolve, Catch Box, Interface, Alexandria. If you consider my screenshot a mockup, then all of these are mockups as well.

Recommendations

Your theme lists full-width as one of it's tags. I can only see that a full-width layout is possible with the portfolio template, but not with other "standard" pages. You may want to note that for users in your readme file.

You are correct, there is no full-width-template, I will remove the tag.

Notes

It's really clear to me that you are an experienced developer with a good knowledge of how to build a responsive theme for WordPress. It would be great to have you extend your theme(s) to also be "accessibility-ready" to be more inclusive for a wider range of audiences. You wouldn't have too much work to do with this theme to acheive that, so I encourage you to consider accessibility!

Yeah I have been thinking about getting into accessible ready mode.

If you do not agree with any of my replies I would encourage you to contact an admin to provide an additional opinion.

Again, thanks a bunch for reviewing my theme

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


23 months ago

#17 @kevinhaig
23 months ago

Please go to ticket #29166 to continue the review.
Because this ticket was closed and then reopened the update loaded into a new ticket.

#18 @kevinhaig
23 months ago

An admin will assign the new ticket to you

#19 @jcastaneda
23 months ago

  • Resolution set to closed-newer-version-uploaded
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.