WordPress.org

Make WordPress Themes

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#11346 closed theme (not-approved)

THEME: Emphaino - 1.0.3

Reported by: SriniG Owned by: life.object
Priority: Keywords: theme-emphaino
Cc: srinig112@…

Description

Emphaino - 1.0.3

The word 'emphaino' means "to show in, to exhibit;" (en, "in," phaino, "to cause to shine"). Let your thoughts, your work, your art shine with Emphaino, a modern, responsive WordPress theme. Clean design, emphasis on content. Customization through the interactive WP customizer interface. Compatible with WordPress 3.5 and above.

Theme URL - http://srinig.com/wordpress/themes/emphaino/
Author URL - http://srinig.com/

SVN - http://themes.svn.wordpress.org/emphaino/1.0.3
ZIP - http://wordpress.org/extend/themes/download/emphaino.1.0.3.zip?nostats=1

Diff with previous version: http://themes.trac.wordpress.org/changeset?old_path=/emphaino/1.0.2&new_path=/emphaino/1.0.3

All previous tickets for this theme: http://themes.trac.wordpress.org/query?col=id&col=summary&col=keywords&col=owner&col=status&col=resolution&keywords=~theme-emphaino&order=id

https://themes.svn.wordpress.org/emphaino/1.0.3/screenshot.png

Change History (9)

#1 @life.object
5 years ago

  • Owner set to life.object
  • Status changed from new to assigned

#2 follow-up: @life.object
5 years ago

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

Review Summary:

  • This is a complete review.

Theme Plugin Test:

Plugin "Theme Check": Pass
Plugin "Debogger": Pass
Plugin "Log Deprecated Notices": None

Required Issues:

  • Flexible height is configured in the custom header arguments but front end does not support flexible height of header.
    • You may set the flex-height false or may support flexible height at front end.
  • When site uses HTTPS and if we're hardcoding the HTTP like in this example
    wp_enqueue_style( 'google_fonts', 'http://fonts.googleapis.com/css?family=PT+Sans:400,700,400italic,700italic|Bree+Serif' );
    

there will be an error. We can either remove http: completely and make it like this

wp_enqueue_style( 'google_fonts', '//fonts.googleapis.com/css?family=PT+Sans:400,700,400italic,700italic|Bree+Serif' );

or use is_ssl()

<?php 
  if (is_ssl()) {
    //action to take for page using SSL
  }
?>

Theme Recommendation:

  • Not-Approved.
  • Please address required issues in the next Theme revision.

Thank you for your submission.

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

Replying to life.object:

Thanks for the review.

  • Flexible height is configured in the custom header arguments but front end does not support flexible height of header.
    • You may set the flex-height false or may support flexible height at front end.

940x140 is the suggested size for the header image. However, if the site title and/or site description is longer than usual and takes up more than one line, the header height adjusts accordingly. The 'flex-height' is there just to cover this kind of a situation, so if the user chooses to have a longer site title, they can upload a header image a little taller than the suggested height. Besides, the header size varies according to the viewport width, so I just want to provide flexibility to the user of the theme to upload a header image with any size they want and just play around till they get it the way they want. I don't want to strictly fix the uploadable header image size to 940x140 or any value.

Be that as it may, I don't see how this can be a requirement and a criteria for not-approving a theme, the theme review guidelines doesn't speak about flex-height or flex-width anywhere. Please correct me if I miss something here.

  • When site uses HTTPS and if we're hardcoding the HTTP like in this example
    wp_enqueue_style( 'google_fonts', 'http://fonts.googleapis.com/css?family=PT+Sans:400,700,400italic,700italic|Bree+Serif' );
    

there will be an error. We can either remove http: completely and make it like this

wp_enqueue_style( 'google_fonts', '//fonts.googleapis.com/css?family=PT+Sans:400,700,400italic,700italic|Bree+Serif' );

or use is_ssl()

<?php 
  if (is_ssl()) {
    //action to take for page using SSL
  }
?>

Thanks for pointing this out, I will fix this and update the theme, but before I upload the updated version, I need clarification on the flex-height issue.

Thanks.

#4 @life.object
5 years ago

@SriniG
Thanks for the feedback,

  • About Flexible Header Height: Let a user decide to choose a header 940x250 without title or short title than the backend is giving proper support but front end is not supporting it. Site Title and/or Site Description and Flexible Headers are two separate things. Introduction of Flexible Headers in 3.5 is providing a freedom to the end user and developer to choose a header of any size.
  • I have not approved the theme as the Admin Panel and Front End are not synchronizing. According to Guidelines Whether implementing required, recommended, or optional features, Themes are required to support proper WordPress core implementation of all included features. In your case, Flexible header is not properly implemented.

Thanks!

#5 @SriniG
5 years ago

I hate to sound stubborn, but please understand that it's not about fixed size, it's more about the ratio. The theme uses background-size: cover; property, so a 1880x280 image would work just as good as 940x140 image. In fact 1880x280 would be better considering retina devices and all that, but again, I don't want to fix the size to 1880x280 because the user should be able to upload 940x140, or even 470x70 image if they want.

I'd love if we can resolve this between ourselves without any outward interference. If you insist, I have no option but to cut down on the flexibility and fix it to 940x140.

Thanks.

#6 follow-up: @life.object
5 years ago

@SriniG

  • My intention is very simple that if a theme supports flexible height header than it should be accordingly just like in TwentyTwelve.
  • I am also waiting for an expert view as you have posted the ticket. I always love to learn new things.
  • There might be another solution that you write a very clear note in the readme file about the header usage and dimensions so the user should be very clear about this.

Thanks

#7 in reply to: ↑ 6 @SriniG
5 years ago

Replying to life.object:

  • My intention is very simple that if a theme supports flexible height header than it should be accordingly just like in TwentyTwelve.

I understand your point of view, but TwentyTwelve is different. Twenty Twelve simply outputs the header image with an img tag, and the header image falls below the site title and tagline. This theme uses the header image as a background for the header and the site title and tagline sit right on top of the image. This theme is more like Twenty Thirteen, the upcoming default theme for WordPress. If you look at Twenty Thirteen, the default header (background) image size is 3200x460 px, and parts of the image goes hidden when you view the site in narrow screens.

I can turn of flex-width and flex-height and fix the header image size to 940x140, but why deny the user an option to crop their image to a larger size like 1880x280 that can work as good as 940x140? After all, the average user doesn't usually resize their image before uploading it as a header image.

  • There might be another solution that you write a very clear note in the readme file about the header usage and dimensions so the user should be very clear about this.

This solution is fine with me. I will make a note of these design constraints with respect the the header image size in the readme file and update the theme. I'll wait for a day or two to see if anyone else has anything to say on this issue

Thanks.

#8 @life.object
5 years ago

@SriniG

Thanks

#9 @SriniG
5 years ago

Updated the theme to version 1.0.4: http://themes.trac.wordpress.org/ticket/11466

The new version enqueues Google webfonts the right way, includes a note about custom header image sizes in the readme file and comes with other minor fixes.

Thanks,
Srini

Note: See TracTickets for help on using tickets.