WordPress.org

Make WordPress Themes

Opened 3 years ago

Closed 2 years ago

#24597 closed theme (not-approved)

THEME: Ilyan – 1.0

Reported by: kunstwerck Owned by: laurelfulford
Priority: new theme Keywords: theme-ilyan
Cc: kunstwerck@…

Change History (10)

#1 follow-up: @themetracbot
3 years ago

  • Owner set to yvette.schroeder
  • Status changed from new to reviewing

#2 @kunstwerck
3 years ago

Hi Yvette,

I'm very much looking forward to your review.
Please, do not hesitate to let me know if you need any further info regarding my theme.

Many thanks,
Selim

#3 in reply to: ↑ 1 @kunstwerck
2 years ago

Replying to themetracbot:
Hi guys - Any feedback on the theme yet?
There is still no activity since months.

Many thanks for your feedback.

#4 @karmatosed
2 years ago

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 urgent review queue. This may mean it still takes time, but should be of more priority. Thanks for your patience.

#5 @laurelfulford
2 years ago

Hello! I can pick up your theme review.

@karmatosed - please assign me this ticket. Thank you! :)

Laurel

#6 @karmatosed
2 years ago

  • Owner changed from yvette.schroeder to laurelfulford

Thanks :)

#7 @laurelfulford
2 years ago

Hello!

I have reviewed your theme against this list and have included feedback below.

The 'Required' items will need to be done before the theme can be approved.

The 'Recommended' items are only recommendations to improve your theme, and do not need to be completed to have your theme approved.

Once the 'Required' items are completed, the updated theme can be uploaded here:

https://wordpress.org/themes/upload/


Required

Code

  • Requiremenu: No PHP or JS errors.
    • In the Customizer, under Ilyan Theme Settings > Front Page Images, I get the following error: Warning: explode() expects parameter 2 to be string, array given in/srv/www/wordpress-default/wp-content/themes/ilyan/inc/customizer.phpon line 239. I was unable to upload more than one carousel image at a time; this might be why?
  • Requirement: Sanitize everything. I found the following spots where escaping is missing - there might be other spots!
    • header.php, line 74 - get_theme_mod() should be escaped when echoed. You could use esc_html() if you didn’t want to allow HTML here, or one of the wp_kses functions if you did want to allow some HTML. Some more information about these escaping functions and others can be found here: https://codex.wordpress.org/Data_Validation
    • inc/custom-header.php, line 72 & 75 - $header_text_colour should be escaped - esc_attr() would work here
    • inc/custom-header.php, line 118 - get_header_textcolor() should be escaped - esc_attr() would work here, too
    • inc/customizer.php, line 40 - get_template_directory_uri() should be escaped with esc_url()
    • inc/customizer.php, 216 - $this->value() should be escaped

--

Core Functionality and Features

  • The theme tags and description must match the what the theme actually does in respect to functionality and design.
    • Please add a ‘Description’ to your theme’s style.css. The description will appear in the Theme Directory below a screenshot of your theme, like with Twenty Fifteen here: https://wordpress.org/themes/twentyfifteen/ It’s a chance to tell folks all about your theme! :)

--

Language

  • Requirement: All theme text strings are to be translatable. I spotted some strings that were not marked for translation:
    • 404.php, line 20: the string 'Back to Homepage' should be marked for translation with your text domain. Eg <?php _e( 'Back to Homepage’, 'ilyan' ); ?>
    • footer.php, line 16 - ‘Theme’ should be marked for translation
    • inc/customizer.php, line 263 - ‘Upload’ should be marked for translation
    • inc/customizer.php, line 266 - ‘Remove all images’ should be marked for translation
  • Requirement: Use a single unique theme slug – as the theme slug appears in style.css. If it uses a framework then no more than 2 unique slugs.
    • inc/extras.php, line 64 - text domain is kunstwerck, but should be ilyan
  • It looks like the languages/ilyan.pot file included with the theme is the default .pot file from _s. This should be removed; the way language files are generated for themes has been changed, so you don’t need to replace it.

--

Licensing

  • Requirement: Declare licenses of any resources included such as fonts or images.
    • License information must be added for the images, Google fonts and Slick font. This information can be added to a new readme.txt file - there’s a good example of formatting for that file here so you can see what information should be included: https://make.wordpress.org/themes/2015/04/29/a-revised-readme/ (Typically name of source, name of license, and link to license). Note that the images bundled with the theme will need to be GPL or have a GPL-compatible license.

--

Plugins

  • Requirement: Don’t do things in a theme considered plugin territory.
    • functions.php, line 242 - 248 - this is commented out, but the function ilyan_title_filter() should be removed from the theme, since it is something more like a plugin should do

--

Selling, credits and links

  • Requirement: If you are a theme shop you should be selling under GPL to be in the WordPress.org repo.
    • Can you please confirm: are all themes you sell GPL?
  • Requirement: If the theme adds a footer credit link, there should only be one (link to WordPress does not count)
    • In footer.php, there are two footer credit links, one to the theme URL and one to the author URL. Please select just one to use (would recommend Author URL, but totally up to you!)

--

Stylesheets and Scripts

  • Requirement: No hard coding of scripts, styles and Favicons unless a browser workaround script. Everything should be enqueued.
    • inc/template-tags.php, line 20, 27, 36, etc. - this JavaScript should be enqueued rather than hardcoded. You may also be able to set a value using wp_localize_script like you do elsewhere in the theme?
  • No minification of scripts or files unless provide original files.
    • The theme uses a number of minifined JavaScript files and one CSS files (the FontAwesome CSS). Please include a non-minified copy of each file in the theme as well.

--

QUESTIONS:

  • Is the function debug_to_console being used in the theme? If not, would strongly recommend removing. Otherwise, it will need to be prefixed with the theme name (like ilyan_debug_to_console). I’ll have to look into how this should be sanitized, too. Just let me know!
  • Is the Slick font actually being used by the theme? I couldn’t spot where, but I might have missed something?

Recommendations

  • Would strongly recommend using a more generic default featured image, or not displaying one at all when a user doesn’t have one. Someone with an established site may have several posts without featured images, so having such an obvious placeholder as an image could deter them from using the theme.
  • functions.php - would recommend removing commented out code, like lines 77 - 82, if not needed
  • header.php, line 59 - minor thing: the_title() does not need echo in front of it - it will already echo the_title() when used. Would recommend removing the echo.
  • rtl.css - this file is empty, so would recommend adding some styles for RTL languages, or just removing.
  • There’s some odd image naming in /fp-img (eg. 1,jpg.jpg and 5,jpg.jpg). Would recommend correcting; they seem to cause broken images in the slideshow.
  • Recommend including license information for all third party scripts and styles in the readme.txt. It looks like this information is available in the individual files, but having all the info in a readme.txt can make it a lot easier for users to see the sources.

VISUAL:

  • Masonry blog layout - this doesn't seem to display as expected. I didn't see any JavaScript errors, so I wonder if it's just a CSS thing: https://cloudup.com/ctVxTNO0tbI
  • Would recommend hiding menu toggle if no menu is assigned - just like the theme already does with the widget toggle.

Next Steps

Once you've completed all of the 'Required' items we can continue with your review. Once the changes are done, just re-upload the theme on the page linked below, it will append to this ticket:

https://wordpress.org/themes/upload/

Also, if you have any questions at all about the above, just let me know!

Thank you for contributing to WordPress!

Laurel

#8 follow-up: @laurelfulford
2 years ago

Hello!

I just wanted to follow up on this review.

I forgot to mention, but it's important to a comment to your active review threads at least once every 7 days, so the thread isn't thought to be inactive. This doesn't mean you need to fix all of the issues in seven days -- just a quick note saying that you're still working on the changes is fine :)

Thanks!

Laurel

#9 in reply to: ↑ 8 @kunstwerck
2 years ago

Hi Laurel,

Thank you very much for your detailed review of my theme.
Unfortunately, so much time has passed since the submission of the theme that I've decided to discontinue further development for this theme because I'm heavily working on another one.

Once again, many thanks - I've certainly learned a lot from your feedback and will let it flow into the new theme.

Best regards,
Selim (aka Kunstwerck)

Replying to laurelfulford:

Hello!

I just wanted to follow up on this review.

I forgot to mention, but it's important to a comment to your active review threads at least once every 7 days, so the thread isn't thought to be inactive. This doesn't mean you need to fix all of the issues in seven days -- just a quick note saying that you're still working on the changes is fine :)

Thanks!

Laurel

#10 @laurelfulford
2 years ago

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

Hi Selim,

Aw, I totally understand! I will close this ticket as 'not-approved'. I'm glad the review will be helpful for future themes! :)

I know it's a bit late for this theme, but the workflow for theme reviews has changed very recently. When a review is abandoned, rather than going into a special list (like this one did), it's returned to the regular 'themes to review' list. Since the abandoned themes will have an older submission date than any other themes in the list, they should be picked up and completed much sooner.

Looking forward to your next theme!

Best,

Laurel

Note: See TracTickets for help on using tickets.