WordPress.org

Make WordPress Themes

Opened 9 months ago

Last modified 7 weeks ago

#33697 approved theme

THEME: Friday – 1.1.1

Reported by: turtlepod Owned by: megane9988
Priority: new theme Keywords: theme-friday
Cc: david@…

Description

Friday - 1.0.0

Simple clean theme with custom logo uploader, layout options, font options, footer-widgets, and full-width page template.

Theme URL - http://genbumedia.com/themes/friday/
Author URL - http://shellcreeper.com/

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

History:

Ticket Summary Status Resolution Owner
#31834 THEME: Friday News Lite – 1.0.0 closed not-approved catchthemes
#33697 THEME: Friday – 1.1.1 approved megane9988

(this ticket)


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

Change History (19)

#1 @themetracbot
9 months ago

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

Friday - 1.0.1

Simple clean theme with custom logo uploader, layout options, font options, footer-widgets, and full-width page template.

Theme URL - http://genbumedia.com/themes/friday/
Author URL - http://shellcreeper.com/

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

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

History:

Ticket Summary Status Resolution Owner
#31834 THEME: Friday News Lite – 1.0.0 closed not-approved catchthemes
#33697 THEME: Friday – 1.1.1 approved megane9988

(this ticket)


https://themes.svn.wordpress.org/friday/1.0.1/screenshot.png

#2 @themetracbot
4 months ago

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

#3 @ronniecoop
4 months ago

The issues below are "required" guidelines which must be followed.

  1. footer.php , menu.php - Use *_url() template tags, rather than bloginfo() equivalents. https://make.wordpress.org/themes/handbook/review/required/#core-functionality-and-features
  1. You should also support up to 3 versions behind the current WordPress version. https://make.wordpress.org/themes/handbook/review/required/ You are supporting up to 4.1.0
  1. You need to include license information in your readme.txt https://make.wordpress.org/themes/handbook/review/required/#licensing Include licenses for esicons fonts, fitvids javascript, the image used to create your screenshot
  1. Wrong template use. Move this code from index.php to top of header.php. Then use get_header() in index.php
    <?php
    <!DOCTYPE html>
    <html <?php language_attributes( 'html' ); ?> class="no-js">
    
    <head>
    <?php wp_head(); ?>
    </head>
    
    <body <?php body_class(); ?>>
    
  1. Translation : Your framework has strings which are not ready for translation
  • /library/functions/strings.php -> All strings
  • /library/modules/full-size-background.php L21 'label' => 'Full Size Background'
  • /library/modules/hide-page-title.php L22 'label' => 'Hide title in single page?'
  • Check other places too

Other Issues

  1. Dont use @import url("base/base.css"); Us wordpress enqueue instead.
  1. Move languagaes folder from assets to the main parent theme
  1. Custom CSS - Wordpress will have Custom CSS in version 4.7, so you can remove it now, or you will need to remove it in future from your theme.
  1. If you keep Custom CSS, then please use wp_add_inline_style() instead of hooking into wp_head in tamatebako_custom_css_wp_head()

You have 7 days to make changes. If you need more time, please post here so that your ticket does not get closed automatically.

#4 @turtlepod
4 months ago

Hi @ronniecoop,
Thank you so much for your time to review the theme. I really appreciate it.

I'll re-check and fix all the points above this weekend and will get back to you on Monday.

best regards,
David.

#5 @themetracbot
4 months ago

  • Summary changed from THEME: Friday – 1.0.1 to THEME: Friday – 1.1.0

Friday - 1.1.0

Simple clean theme with custom logo uploader, layout options, font options, footer-widgets, and full-width page template.

Theme URL - http://genbumedia.com/themes/friday/
Author URL - http://shellcreeper.com/

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

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

History:

Ticket Summary Status Resolution Owner
#31834 THEME: Friday News Lite – 1.0.0 closed not-approved catchthemes
#33697 THEME: Friday – 1.1.1 approved megane9988

(this ticket)


https://themes.svn.wordpress.org/friday/1.1.0/screenshot.png

#6 @turtlepod
4 months ago

Hi @ronniecoop,
Friday is updated to version 1.1.0
Here are some information about this update/your review:

1) Use *_url() template tags, rather than bloginfo() equivalents.

the only blog info used is get_bloginfo( 'name' ) (there's no template function equivalents for that ?).
All url such as home_url() already use correct template function.

2) You should also support up to 3 versions behind the current WordPress version.

3 version would be 4.3 ; 4.4 ; 4.5, not wp 1 - 2 - 3 (that would be ridiculus, right?)
Please confirm this with other reviewer if you are unsure.

3) License information

License added in readme.txt
Thank you, I completly forgot about this.

4) Wrong template use.

Nothing wrong here. all is correct (?).
It is uncommon but acceptable.
I even ask another reviewer about this, and it is OK.
If you are unsure, you can consult with (senior) reviewer (?)

5) Translation

library/modules/full-size-background.php
library/modules/hide-page-title.php

This are fallback strings. the actual string loaded in added via theme args, so it will be translated properly.
The "/library/functions/strings.php" are only for "core" tamatebako. full-size-background and hide-page-title are "modules", and all translated strings should be added via support args. This is because "modules" only loaded if theme explicitly add support for it/use the module (optional).
check "includes/background.php" and "includes/layouts.php" to see that the strings are properly translate-able.

6) @import url("base/base.css");

Actually, that line is not even loaded (?)

7) Move languagaes folder from assets to the main parent theme

Not a requirements.
I'll keep the current structure.

8) Custom CSS Module

I remove this from friday. in tamatebako future update I will also remove it completely in favor for wp 4.7 custom css.
Sidenote: wp_head is perfectly fine for this too (As far as i know?)

Thank you so much for your time to review this.
I really appreciate your valuable time.

I'll look forward for more information and review from you.
And let me know if there's anything I need to explain further.

best regards,
David.

Last edited 4 months ago by turtlepod (previous) (diff)

#7 @ronniecoop
3 months ago

2) You should also support up to 3 versions behind the current WordPress version.
3 version would be 4.3 ; 4.4 ; 4.5, not wp 1 - 2 - 3 (that would be ridiculus, right?)
Please confirm this with other reviewer if you are unsure.

You misunderstand. You are supporting upto 4.1.0 You should only support till 4.3.0
For example, you can use function_exists only for functions introduced in 4.3.0, not before that.

5) Translation
library/modules/full-size-background.php
library/modules/hide-page-title.php
This are fallback strings. the actual string loaded in added via theme args, so it will be translated properly.
The "/library/functions/strings.php" are only for "core" tamatebako. full-size-background and hide-page-title are "modules", and all translated strings should be added via support args. This is because "modules" only loaded if theme explicitly add support for it/use the module (optional).
check "includes/background.php" and "includes/layouts.php" to see that the strings are properly translate-able.

I'll leave this up to the key reviewer when they makes the final review (for strings added via support args), however I'll strongly recommend you make them translatable, as it is very easy to miss them. For example:
in modules/back-compat.php, you have strings, but you forgot to add translatable strings in support args inside friday_theme_setup in $back_compat_args

Also, what is the point of core fallback strings, if you are going to provide translatable strings every time using support args. It defeats the purpose of having fallback strings.

Also, the core strings in library/functions/strings.php need to be translatable.
I could not find any alternative function for these. These are directly called using tamatebako_string() which simply picks up the non translatable string, and throws it back.

#8 @ronniecoop
3 months ago

Sorry I misunderstood. Forget about the WP version requirement I mentioned above. I realized you are simply using it as a 'minimum requirement' and not supporting older versions.

#9 @turtlepod
3 months ago

Hi @ronniecoop,
Thank you for your time to reply.

So, I think whats left is to wait for key reviewer to check.

Just to clarify:
1) the "Friday" text string in the back-compat (functions.php) is not translatable by choice. It's a name ( not "day" in this context ). and I don't want it to be translatable. We don't translate "Open Sans" or "WordPress" for the same reason.

2) The reason translation in module is added via args, is because a theme can choose to not use the module. I don't want the translator to translate the text string that's not even used in the theme.

I hope that clears up the confusion. But let me know if you need more info.

And thank you for behaving as a PRO reviewer. I really appreciate it.
It's been a pleasure to communicate with you.

(a lot of reviewer seem to think they are "above" theme author, and sometimes get cocky, don't listen like a sheriff or police waiving their gun or something/hero complex stuff)

best regards,
David.

Last edited 3 months ago by turtlepod (previous) (diff)

#10 @grapplerulrich
3 months ago

  • Owner ronniecoop 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.

#11 @turtlepod
3 months ago

thank you @grapplerulrich.

any help appreciated, this time it took too long.

#12 @themetracbot
3 months ago

  • Owner set to megane9988

#13 @megane9988
3 months ago

Hi @turtlepod .

I checked your theme.

one Issue fix please.

1) Your theme has two text-domain. friday and nokonoko.

friday/includes/utility.php:

6 /* === CUSTOM CSS === */
7 $custom_css_args = array(
8: 'title' => _x( 'Custom CSS', 'customizer', 'nokonoko' ),
9: 'label' => _x( 'Custom CSS', 'customizer', 'nokonoko' ),

More than one text-domain is being used in this theme. This means the theme will not be compatible with WordPress.org language packs.
The domains found are friday, nokonoko

please go to https://wordpress.org/themes/upload and reupload a new one. Change the version in style.css.

#14 @megane9988
3 months ago

One more Issue.

readme.txt

You should replace first line '=== NokoNoko ===' to '=== Friday === '.

#15 @themetracbot
3 months ago

  • Summary changed from THEME: Friday – 1.1.0 to THEME: Friday – 1.1.1

Friday - 1.1.1

Simple clean theme with custom logo uploader, layout options, font options, footer-widgets, and full-width page template.

Theme URL - http://genbumedia.com/themes/friday/
Author URL - http://shellcreeper.com/

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=friday/1.1.0&new_path=friday/1.1.1

History:

Ticket Summary Status Resolution Owner
#31834 THEME: Friday News Lite – 1.0.0 closed not-approved catchthemes
#33697 THEME: Friday – 1.1.1 approved megane9988

(this ticket)


https://themes.svn.wordpress.org/friday/1.1.1/screenshot.png
Theme Check Results:

  • RECOMMENDED: No reference to add_theme_support( "custom-header", $args ) was found in the theme. It is recommended that the theme implement this functionality if using an image for the header.
  • RECOMMENDED: Tags: is either empty or missing in style.css header.

#16 @turtlepod
3 months ago

Thank you @megane9988.
I've updated the theme as requested.

Please check the update.

-- David.

#17 @megane9988
3 months ago

  • Status changed from reviewing to approved

@turtlepod grate job.

Thank you for your upload.
I think OK.

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


3 months ago

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


7 weeks ago

Note: See TracTickets for help on using tickets.