WordPress.org

Make WordPress Themes

Opened 3 years ago

Closed 3 years ago

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

THEME: Altitude Lite - 1.0.0.3

Reported by: cyberchimps Owned by: Frank Klein
Priority: previously reviewed Keywords: theme-altitude-lite
Cc: development@…, Frank, Klein

Description

Altitude Lite - 1.0.0.1

Altitude Lite is a responsive sleek mobile first design embodying sheer beauty. This theme features the theme customizer for live theme options, a beautiful parallax header image, custom logo, sharp typography, and looks amazing on mobile devices.

Theme URL - http://www.cyberchimps.com/altitude/
Author URL - http://cyberchimps.com/

SVN - https://themes.svn.wordpress.org/altitude-lite/1.0.0.1
ZIP - https://wordpress.org/themes/download/altitude-lite.1.0.0.1.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=/altitude-lite/1.0.0.0&new_path=/altitude-lite/1.0.0.1

History:

Ticket Summary Status Resolution Owner
#20334 THEME: Altitude Lite - 1.0.0.0 closed not-approved ferdieyard
#21443 THEME: Altitude Lite - 1.0.0.3 closed closed-newer-version-uploaded Frank Klein

(this ticket)

#21607 THEME: Altitude Lite - 1.0.0.4 closed live Frank Klein
#35366 THEME: Altitude Lite – 1.0.0.5 closed live themetracbot
#36945 THEME: Altitude Lite – 1.1 closed live themetracbot
#42248 THEME: Altitude Lite – 1.2 closed live themetracbot
#44918 THEME: Altitude Lite – 1.3 closed live themetracbot


https://themes.svn.wordpress.org/altitude-lite/1.0.0.1/screenshot.png

Change History (27)

#1 @emiluzelac
3 years ago

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

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


3 years ago

#3 @limestreet
3 years ago

Hi,
theme is realy nice and I have found only two required issues:

  1. Theme is tagged "rtl-language-support" but there is no real RTL support. More info: http://codex.wordpress.org/Right-to-Left_Language_Support

Please remove tag from style.css header or add styling to rtl.css

  1. Please check function altitude_sanitize_sidebar in customizer.php - it does nothing.

Best regards
Patryk

#4 follow-up: @Frank Klein
3 years ago

Thanks to @limestreet for the review! I looked the theme over and found a couple more issues to take care of:

Required

template-blog.php

  • This template should be removed, as the Reading Settings and Template Hierarchy make this template unnecessary.

header.php

  • Please escape get_theme_mod( 'altitude_home_button_text' ) with esc_html().

functions.php

  • get_altitude_sidebar() should be renamed to altitude_get_sidebar(), so that it is correctly prefixed.
  • altitude_register_styles() should be removed, as the styles can be directly enqueued in altitude_load_styles() and altitude_admin_scripts().
  • altitude_load_styles(): Please remove the $hook parameter and hook this function to wp_enqueue_scripts to enqueue these fonts on the front end.

comments.php

  • You added HTML5 support for the comment list in functions.php, so having a render callback in wp_list_comments() is redundant.

layouts/editor-styles.css

  • The styles in this file do not correspond to the front end, please remove this file or correct the styles.

inc/admin.php

  • As the code in this file is no longer needed, please remove this file entirely, along with the css/admin.css file and the now also unnecessary CyberChimps font in font/.

#5 in reply to: ↑ 4 @limestreet
3 years ago

I'm trying to do my best, but is not easy :)

#6 @themetracbot
3 years ago

  • Summary changed from THEME: Altitude Lite - 1.0.0.1 to THEME: Altitude Lite - 1.0.0.2

Altitude Lite - 1.0.0.2

Altitude Lite is a responsive sleek mobile first design embodying sheer beauty. This theme features the theme customizer for live theme options, a beautiful parallax header image, custom logo, sharp typography, and looks amazing on mobile devices.

Theme URL - http://www.cyberchimps.com/altitude/
Author URL - http://cyberchimps.com/

SVN - https://themes.svn.wordpress.org/altitude-lite/1.0.0.2
ZIP - https://wordpress.org/themes/download/altitude-lite.1.0.0.2.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=/altitude-lite/1.0.0.1&new_path=/altitude-lite/1.0.0.2

History:

Ticket Summary Status Resolution Owner
#20334 THEME: Altitude Lite - 1.0.0.0 closed not-approved ferdieyard
#21443 THEME: Altitude Lite - 1.0.0.3 closed closed-newer-version-uploaded Frank Klein

(this ticket)

#21607 THEME: Altitude Lite - 1.0.0.4 closed live Frank Klein
#35366 THEME: Altitude Lite – 1.0.0.5 closed live themetracbot
#36945 THEME: Altitude Lite – 1.1 closed live themetracbot
#42248 THEME: Altitude Lite – 1.2 closed live themetracbot
#44918 THEME: Altitude Lite – 1.3 closed live themetracbot


https://themes.svn.wordpress.org/altitude-lite/1.0.0.2/screenshot.png

#7 @limestreet
3 years ago

  • Cc Frank Klein added

Hi, thanks for upload.

Looking at "diff" I see that some issues are still not fixed:

comments.php
You added HTML5 support for the comment list in functions.php, so having a render callback in wp_list_comments() is redundant.

layouts/editor-styles.css
The styles in this file do not correspond to the front end, please remove this file or correct the styles.

inc/admin.php
As the code in this file is no longer needed, please remove this file entirely, along with the css/admin.css file and the now also unnecessary CyberChimps font in font/.

customizer.php
Sanitization for sidebar option.
function altitude_sanitize_sidebar( $input ){
if( $input )
return $input;
else
return 'right';
}

#8 @cyberchimps
3 years ago

Thanks for the quick review. Let's discus those other points.

comments.php:

The render callback is actually added for the customization work. So that when some structural changes to the comments HTML needed that can be done.

layouts/editor-styles.css:

Yes, this is not for the frontend. This is for the post/page edit page.


admin.php:

admin.php is actually used for some purpose. If you check the file you can find that it is used to display link back to CyberChimps and from that file admin.css file is included where we have styles for admin end.

customizer.php

Here it is checked whether any value for that option exists or not. If not that means that option is not saved, so it should be set to default. So that's what it is done here, set the value to default 'right'.

#9 @limestreet
3 years ago

  • Status changed from reviewing to approved

Good :)
So I think can approve.

--
Patryk

#10 @cyberchimps
3 years ago

Thank you so much.

#11 @emiluzelac
3 years ago

  • Status changed from approved to reopened

I don't like doing this, but no choice. admin.php must be removed before this theme goes live. As noted in your other ticket, authors can include the link in backend, but only from theme options.

#12 @emiluzelac
3 years ago

P.S. Some of the @Frank Klein's notes are not addressed, please go over one more time if you can.

@limestreet thanks for the help, let's leave the final approval for Frank :)

Last edited 3 years ago by emiluzelac (previous) (diff)

#13 @cyberchimps
3 years ago

@Emil : Okay, i will remove admin.php. But i have explained other points mentioned by Frank. So please let me know whether they are okay or not. Then i will upload a new version with all the required fixes.

#14 @emiluzelac
3 years ago

Frank is the mentor, therefore he will handle the rest. The only reason I interfered is because ticket was approved and Frank didn't have a chance to go over one more time. Tickets are marked live fast nowadays and I didn't want to ship a theme with issues.

#15 @emiluzelac
3 years ago

  • Cc Frank+Klein added; Frank Klein removed

#16 @emiluzelac
3 years ago

  • Cc Frank Klein added; Frank+Klein removed

Weird, Cc: keeps adding , after Frank ;)

#17 @cyberchimps
3 years ago

That's really fine. I will then just wait for Frank's comment on those points.

#18 @Frank Klein
3 years ago

comments.php: The render callback is actually added for the customization work. So that when some structural changes to the comments HTML needed that can be done.

You can have a comment render callback function, that is no problem. However in that case you need to remove comment-list from add_theme_support( 'html5' ).

When you are adding HTML5 support for an element, you are indicating that you want to use the markup provided by Core. So indicating you want to use Core's markup and then having your own callback function is redundant. Its either HTML5 support, or your custom function.

layouts/editor-styles.css: Yes, this is not for the frontend. This is for the post/page edit page.

That is okay, but this is how the front end looks: https://cloudup.com/izSTYsBh9Zh
And this is how the back end looks: https://cloudup.com/ivNI177tq1g

So you see that in the admin it's the wrong typography, spacing, sizes, etc. So when using editor styles, they either should have some resemblance to the front end, or you should not use editor styles.

customizer.php Here it is checked whether any value for that option exists or not. If not that means that option is not saved, so it should be set to default. So that's what it is done here, set the value to default 'right'.

You don't understand your own code correctly. Here's the function, let's go over it line by line:

function altitude_sanitize_sidebar( $input ){
	if( $input )
		return $input;
	else
		return 'right';
}
  1. $input is the value provided by the Customizer. This is unsafe data provided by the user.
  2. If $inputexists, save $input to the database with no sanitization whatsoever.
  3. Else save right.

This is how the function should be written:

function altitude_sanitize_sidebar( $input ){
	if ( in_array ( $input, array( 'right', 'left' ), true ) ) {
		return $input;
	} else {
		return 'right';
	}

With this approach, you are whitelisting the possible values that can be returned by the Customizer, and you are only saving values that correspond to the possibilities offered by your option.

#19 @themetracbot
3 years ago

  • Summary changed from THEME: Altitude Lite - 1.0.0.2 to THEME: Altitude Lite - 1.0.0.3

Altitude Lite - 1.0.0.3

Altitude Lite is a responsive sleek mobile first design embodying sheer beauty. This theme features the theme customizer for live theme options, a beautiful parallax header image, custom logo, sharp typography, and looks amazing on mobile devices.

Theme URL - http://www.cyberchimps.com/altitude/
Author URL - http://cyberchimps.com/

SVN - https://themes.svn.wordpress.org/altitude-lite/1.0.0.3
ZIP - https://wordpress.org/themes/download/altitude-lite.1.0.0.3.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=/altitude-lite/1.0.0.2&new_path=/altitude-lite/1.0.0.3

History:

Ticket Summary Status Resolution Owner
#20334 THEME: Altitude Lite - 1.0.0.0 closed not-approved ferdieyard
#21443 THEME: Altitude Lite - 1.0.0.3 closed closed-newer-version-uploaded Frank Klein

(this ticket)

#21607 THEME: Altitude Lite - 1.0.0.4 closed live Frank Klein
#35366 THEME: Altitude Lite – 1.0.0.5 closed live themetracbot
#36945 THEME: Altitude Lite – 1.1 closed live themetracbot
#42248 THEME: Altitude Lite – 1.2 closed live themetracbot
#44918 THEME: Altitude Lite – 1.3 closed live themetracbot


https://themes.svn.wordpress.org/altitude-lite/1.0.0.3/screenshot.png

#20 @cyberchimps
3 years ago

Thanks Frank for the detailed reply. Uploaded a new version with all the fixes. Hope it should be fine now.

#21 @Frank Klein
3 years ago

  • Owner changed from limestreet to Frank Klein
  • Status changed from reopened to reviewing

Looks good to me.

#22 @Frank Klein
3 years ago

  • Status changed from reviewing to approved

#23 @Frank Klein
3 years ago

Approved too fast. This is how the theme looks after activation (no navigation menu set): https://cloudup.com/ijS5Yy_aSUh

Can you please look into this?

#24 @Frank Klein
3 years ago

I'm using the Theme Test Data on this site by the way.

#25 @cyberchimps
3 years ago

I fixed that and uploaded, but it created another thread : https://themes.trac.wordpress.org/ticket/21607

#26 @karmatosed
3 years ago

  • Status changed from approved to reopened

#27 @karmatosed
3 years ago

  • Resolution set to closed-newer-version-uploaded
  • Status changed from reopened to closed

Newer version with fix uploaded here: https://themes.trac.wordpress.org/ticket/21607

Note: See TracTickets for help on using tickets.