WordPress.org

Make WordPress Themes

Opened 4 months ago

Closed 4 months ago

#43504 closed theme (live)

THEME: Mediumm – 1.1.5

Reported by: jojoee Owned by: rabmalin
Priority: previously reviewed Keywords: theme-mediumm
Cc: inid3a@…

Change History (15)

#1 follow-up: @jojoee
4 months ago

Hi @excellentdynamics,
I'm not sure, why it create new ticket for the theme.
Could you help me to check please ?

Old ticket: https://themes.trac.wordpress.org/ticket/41990
New ticket: https://themes.trac.wordpress.org/ticket/43504

Update

@excellentdynamics
I've fixed all reported issues,
except point 5 (widget design) can I keep it in that way ?

  • Support custom body background color and image
  • Support custom header color and header background image
  • Change text-domain from "medm" to "mediumm"
  • Fixed, un-scale featured image width
  • Fixed, fluid width Youtube video embeds (iframe) in content
  • Fixed, article category tag overflow (listing page)
  • Fixed, mobile menu centering
  • Fixed, Jetpack tiled gallery
  • Fixed, text beside image should be float (single post)
  • Fixed, text overflow on article's content (single post)
  • Fixed, text overflow on article's summary (listing page)

and also check "Theme Check" and "Deprecated Calls" plugins

@joyously,
I've implement some that posted by you, and also post
it on https://github.com/jojoee/medium-clone/issues/3
in case you want to track or add something more

Best

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

#2 in reply to: ↑ 1 @excellentdynamics
4 months ago

Hi

Yes, everything is good. I approved 1.0.5

Best

Replying to jojoee:

Hi @excellentdynamics,
I'm not sure, why it create new ticket for the theme.
Could you help me to check please ?

Old ticket: https://themes.trac.wordpress.org/ticket/41990
New ticket: https://themes.trac.wordpress.org/ticket/43504

Update

@excellentdynamics
I've fixed all reported issues,
except point 5 (widget design) can I keep it in that way ?

  • Support custom body background color and image
  • Support custom header color and header background image
  • Change text-domain from "medm" to "mediumm"
  • Fixed, un-scale featured image width
  • Fixed, fluid width Youtube video embeds (iframe) in content
  • Fixed, article category tag overflow (listing page)
  • Fixed, mobile menu centering
  • Fixed, Jetpack tiled gallery
  • Fixed, text beside image should be float (single post)
  • Fixed, text overflow on article's content (single post)
  • Fixed, text overflow on article's summary (listing page)

and also check "Theme Check" and "Deprecated Calls" plugins

@joyously,
I've implement some that posted by you, and also post
it on https://github.com/jojoee/medium-clone/issues/3
in case you want to track or add something more

Best

#3 @rabmalin
4 months ago

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

#4 @rabmalin
4 months ago

Hello,

Please fix following issues are re-upload your theme.

Please respond within 7 days. If there is no response for 7 days, ticket will be closed as not-approved.

Issues

  • REQUIRED: Include all scripts and resources it uses rather than hot-linking. The exception to this is Google Fonts.
  • REQUIRED: Make sure excerpt_more filter does not affect admin side. See Twenty Seventeen theme for reference.
  • REQUIRED: Use home_url() rather than get_home_url(). Also you need to escape home_url() with esc_url().
  • REQUIRED: The main issue in the theme is PHP compatibility. You are allowed to support whatever version you like. But there should be graceful failing in older PHP version like PHP 5.2. There should be no PHP fatal error.
  • REQUIRED: No hard coding of script and style files. All scripts, stylesheets, Google Fonts, etc should be enqueued properly. https://make.wordpress.org/themes/handbook/review/required/#stylesheets-and-scripts

Please fix above issues and I will review theme in more detail.

#5 @themetracbot
4 months ago

  • Summary changed from THEME: Mediumm – 1.1.1 to THEME: Mediumm – 1.1.2

#6 @jojoee
4 months ago

Hi, @rabmalin thank you for reviewing.

There is an update

  • Include all scripts and resources it uses rather than hotlinking. The exception to this is Google Fonts.

Done, using Web Font Loader from local instead of CDN
https://github.com/jojoee/medium-clone/commit/a151d0189c1e01a357daa3829aea7b199a960733

  • Make sure excerpt_more filter does not affect admin side.

Done, add condition into "excerpt_more" filter to make sure it will not affect admin side
https://github.com/jojoee/medium-clone/commit/54c63642761ef7181e3b8865580828da8843d8f8

  • Use home_url() rather than get_home_url(). Also you need to escape home_url() with esc_url().

Done, using esc_url + home_url instead of get_home_url
https://github.com/jojoee/medium-clone/commit/edba92fe3b5b4015d4ba9f7aea680ac76dbc3cf0

  • No hard coding of script and style files.

I'm checked but I'm not sure where it's incorrect.
Please help me to specific a location if it still incorrect.

  • PHP compatibility

Due to I'm using PHP namespace feature.
So, the theme cann't support PHP below 5.3.
Is there anyway to not support php below 5.3 ?

Best,
Nathachai Thongniran

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

#7 @rabmalin
4 months ago

Issues

  • REQUIRED: templates/comments.php L7 - Missing singular placeholder, needed for some languages. See https://codex.wordpress.org/I18n_for_WordPress_Developers#Plurals See Twenty Seventeen for reference.
  • REQUIRED: Remove `jquery.js.
  • REQUIRED: main.js - Still some codes are minified in this file. Please check it.
  • REQUIRED: The main issue in the theme is PHP compatibility. You are allowed to support whatever version you like. But there should be graceful failing in older PHP version like PHP 5.2. There should be no PHP fatal error. - I dont have idea how to do it exactly. I will inform you I find the correct approach. In the meanwhile please do research regarding that.
  • REQUIRED: Theme tag custom-background missing style.css.
  • REQUIRED: Theme tag custom-header missing style.css.
  • REQUIRED: Theme tag custom-menu missing style.css.
  • REQUIRED: Remove post-formats theme tag and related code as theme does not seem to implement Post Formats.
  • REQUIRED: Translation issue - utils.php L89; content-search.php L30; Please check thoroughly.
  • REQUIRED: <?php get_template_part( 'templates/head' ); ?> - Styles are directly added in header. Pleae use wp_head or other alternative.

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


4 months ago

#9 @themetracbot
4 months ago

  • Summary changed from THEME: Mediumm – 1.1.2 to THEME: Mediumm – 1.1.3

#10 @jojoee
4 months ago

Done

  • Remove unused jquery.js
  • main.js have some codes are minified in this file
  • Theme tag custom-background missing style.css
  • Theme tag custom-header missing style.css
  • Theme tag custom-menu missing style.css
  • Remove post-formats theme tag
  • Missing translation on utils.php L89; content-search.php L30;

Researching

  • PHP 5.2 compatibility

Need suggestion

I'm checked it but not sure where am I missing, could you please give me more details about it ?

  • <?php get_template_part( 'templates/head' ); ?> - Styles are directly added in header. Pleae use wp_head or other alternative.
  • templates/comments.php L7 - Missing singular placeholder, needed for some languages

#11 @themetracbot
4 months ago

  • Summary changed from THEME: Mediumm – 1.1.3 to THEME: Mediumm – 1.1.4

#12 @grapplerulrich
4 months ago

@rabmalin asked a second review on the theme. I few notes

  • Like mentioned above "REQUIRED: The main issue in the theme is PHP compatibility. You are allowed to support whatever version you like. But there should be graceful failing in older PHP version like PHP 5.2. There should be no PHP fatal error." There needs to be a check that the user is running PHP 5.3 or above other the theme should not activate and bring the users site down.
  • If you are going to use namespaces then use them in every file and not just some of them.

#13 @themetracbot
4 months ago

  • Summary changed from THEME: Mediumm – 1.1.4 to THEME: Mediumm – 1.1.5

#14 @jojoee
4 months ago

Current version: 1.1.5

Update from 1.1.4

  • Refactor, namespace system
  • Refactor, change code style (using PhpStorm default WordPress code style)
  • Check PHP version on theme activation (theme can not activated, if running on PHP version below 5.3.0)

Update from latest reviews by @rabmalin on 1.1.3

  • Fixed, Remove unused jquery.js
  • Fixed, main.js have some codes are minified in this file
  • Fixed, Theme tag custom-background missing style.css
  • Fixed, Theme tag custom-header missing style.css
  • Fixed, Theme tag custom-menu missing style.css
  • Fixed, Remove post-formats theme tag
  • Fixed, Missing translation on utils.php L89; content-search.php L30;
  • Fixed, PHP compatibility by checking PHP version on theme activation as @grapplerulrich suggested.

On these 2 points below, I'm not sure where am I missing, please suggest if it still need to be fixed.

  • <?php get_template_part( 'templates/head' ); ?> - Styles are directly added in header. Pleae use wp_head or other alternative.
  • templates/comments.php L7 - Missing singular placeholder, needed for some languages

#15 @rabmalin
4 months ago

  • Resolution set to live
  • Status changed from reviewing to closed

Approving and setting live. It will take around 1 hour for theme to appear in the directory. In the next update please fix following issues.

  • Remove post-formats theme tag and related code as theme does not seem to implement Post Formats. -> You should remove this also. add_theme_support( 'post-formats', array( 'aside', 'gallery', 'link', 'image', 'quote', 'video', 'audio' ) );
  • /templates/header.php - Here you have added dynamic style, check $custom_header_style. You should use either wp_head hook or https://developer.wordpress.org/reference/functions/wp_add_inline_style/ to add dynamic CSS.
Note: See TracTickets for help on using tickets.