WordPress.org

Make WordPress Themes

Attachments (1)

rational-start.0.2.zip (619.2 KB) - added by jeremyHixon 3 years ago.
Modified with all the required and 1 of the recommended changes.

Download all attachments as: .zip

Change History (60)

#1 @themetracbot
3 years ago

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

#2 @jeremyHixon
3 years ago

If you have any questions, please, let me know. Thanks.

#3 @jayantisolanki
3 years ago

Hello @jeremyHixon,

Anything marked REQUIRED needs a resolution. Anything marked RECOMMENDED is suggested a fix but not a grounds for failing.

REQUIRED

  • All strings are to be translatable. This means wrapping all strings in a gettext function and loading text domain via load_theme_textdomain() See Twenty Fifteen for code example.
  • search.php
  • index.php
  • home.php
  • header.php
  • archive.php
  • 404.php
  • ...and many more. Please check all the files.

RECOMMENDED

  • Possible variable $recent_post found in translation function in widgets.php. Translation function calls must NOT contain PHP variables. ( $recent_postpost_title?, 'rationalstart' )
  • Possible variable $recent_comment found in translation function in widgets.php. Translation function calls must NOT contain PHP variables. ( get_the_title( $recent_comment->comment_post_ID ), 'rationalstart' )
  • Possible variable $link_title found in translation function in widgets.php. Translation function calls must NOT contain PHP variables. ( $link_title, 'rationalstart' )
  • Possible variable $link_title found in translation function in widgets.php. Translation function calls must NOT contain PHP variables.
  • 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.
  • No reference to add_theme_support( "custom-background", $args ) was found in the theme. If the theme uses background images or solid colors for the background, then it is recommended that the theme implement this functionality.

CONCLUSION

Please fix above issues and upload new version of your theme, I'll leave this ticket open for another 7 days. Feel free to ask if you have any questions.

#4 @jeremyHixon
3 years ago

Awesome. Thank you for the feedback. I'll get right on these.

@jeremyHixon
3 years ago

Modified with all the required and 1 of the recommended changes.

#5 @jeremyHixon
3 years ago

Attached a new zip with required changes made and custom background support added.

#6 @jayantisolanki
3 years ago

Please upload new version of your theme here

#7 @jeremyHixon
3 years ago

Apologies. I'll do that. Do all the theme parameters need to be the same, like name, version, etc... in order for it to be updated here?

#8 @jayantisolanki
3 years ago

Yes @jeremyHixon, all the theme parameters are same except theme version.

Example...

Current Version : 0.1
Updated Theme Version : 0.2

#10 @jeremyHixon
3 years ago

There we go. Thank you for helping me understand the system.

#11 @jayantisolanki
3 years ago

  • Status changed from reviewing to approved

Hi @jeremyHixon,

Thank you. Everything looks clear, marking theme as APPROVED.

#12 @djrmom
3 years ago

  • Cc djrmom added
  • Keywords changed from theme-rational-start, accessibility-ready to theme-rational-start accessibility-ready
  • Status changed from approved to reopened

#13 @djrmom
3 years ago

  • Cc joedolson added

A few required issues to fix before going live:

  • Customizer use for theme settings is required, you need to switch from your theme options page to customizer
  • The text-domain for translations must be exactly the theme slug "rational-start" rather than "rationalstart"
  • google fonts need to be enqueue rather than use @import within the stylesheet, they also need to be protocol relative:
    wp_enqueue_style('rational-start-fonts','//fonts.googleapis.com/css?family=Raleway:400,700|Inconsolata|Source+Sans+Pro');
    
  • The default in the footer social widget should not contain links to your social profiles or have a title referencing the theme name.
  • options in function rational_head_cleaner are plugin territory, please remove the functions and options
  • open graph is also plugin territory - function rational_social_head
  • all text strings need to be translation ready - check theme_setup.php ln 47, widget names starting at ln 107 - text strings in theme options also need to be translation ready so be sure to check those when converting to customizer
  • add_image_size needs to be prefixed with theme slug, ex:
    add_image_size( 'rational-start-post-full', 1400, 577, true );
    
  • custom scripts and styles need to have their handle prefixed with theme slug, third party scripts can use the name of the script, ex:
    wp_enqueue_style( 'rational-start-theme-style' );
    
  • You cannot bundle a copy of jquery, you must use core. You may have an option to enable the cdn version but it must to disabled by default
  • tags in style.css, see https://make.wordpress.org/themes/handbook/review/required/theme-tags/:
    • the colors can only the main color scheme - green, orange, red, yellow - should be removed
    • three-columns, four-columns, flexible-header, featured-image-header - these don't appear to be supported

You also need to have a separate accessibility review since you have included that tag. (cc @joedolson)

#14 @djrmom
3 years ago

One more thing, you need to include a copyright attribution for your theme and a list of bundled resources (readme.txt is a good place) with their copyright/attribution, license and source link, see https://make.wordpress.org/themes/2014/07/08/proper-copyrightlicense-attribution-for-themes/ for an example.

#15 @jeremyHixon
3 years ago

Thank you @djrmom, I'll get started.

#16 @joedolson
3 years ago

I'll work on the accessibility audit after the next version is uploaded. Thanks for cc'ing me!

#17 @jeremyHixon
3 years ago

Thank you @djrmom. I have a quick question though.

options in function rational_head_cleaner are plugin territory, please remove the functions and options

Is that to say that the remove_action function is plugin level? I didn't see that notice in any of the documentation.

#18 @djrmom
3 years ago

Hi @jeremyHixon,

There is no problem with the remove_action function itself, but you are not allowed to remove non-presentational hooks, see https://make.wordpress.org/themes/handbook/review/required/explanations-and-examples/#code

#19 @jeremyHixon
3 years ago

Ah, thank you @djrmom, I must have missed that line. I'll make note for the future.

#20 @themetracbot
3 years ago

  • Summary changed from THEME: Rational Start – 0.2 to THEME: Rational Start – 1.0

#21 @djrmom
3 years ago

  • Cc greenshady added

Hi @jeremyHixon,

You will need to remove jquery.min.js and this line:

wp_register_script( 'jquery', get_stylesheet_directory_uri() . '/js/jquery.min.js', false, '1.11.2' );

The above does not do anything because jquery is already registered, but if it did work would not be allowed. You need to use core jquery when cdn option is not checked.

I don't think the "head cleanup" options are allowed, even as an opt-in but I will cc an admin to check on that.

Also, the .codekit-cache directory got included in your zip file.

@greenshady, can you comment on the profile, pingback, and rss feeds settings in https://themes.svn.wordpress.org/rational-start/1.0/inc/theme_settings.php, they are opt-in and disabled by default

#22 @greenshady
3 years ago

Please note that your theme is only compatible with PHP versions 5.5.0+ in the description or readme. The boolval() function is not available in older versions.

The Pingback and RSS Feeds options are plugin territory and should be removed.

Also, the theme must use the jQuery that's packaged with core WP. So, the Google CDN for jQuery option needs to be removed. This is also plugin territory.

#23 @jeremyHixon
3 years ago

Thanks for the feedback @djrmom and @greenshady. I'm on it.

#24 @themetracbot
3 years ago

  • Summary changed from THEME: Rational Start – 1.0 to THEME: Rational Start – 1.0.1

#25 @jeremyHixon
3 years ago

@djrmom and @greenshady, changes made. Thank you for the assistance.

#26 @djrmom
3 years ago

Hi @jeremyHixon,

I am very behind due to vacation and general business but should be able to review by the end of the week.

Thanks,
Jenny

#27 @djrmom
3 years ago

  • Cc djrmom joedolson greenshady removed

Hi @jeremyHixon,

This looks good, please remove jquery.min.js from the js folder and then it should be ready.

However, it looks like @joedolson still needs to do the accessibility review portion.

Thanks,
Jenny

#28 @djrmom
3 years ago

  • Cc djrmom added

#29 @djrmom
3 years ago

  • Cc joedolson added

#30 @joedolson
3 years ago

Thanks for the ping. I'll get on this asap.

#31 @joedolson
3 years ago

Before I start the accessibility review, your theme is throwing a database error due to improper use of $wpdb. In /inc/widgets.php, you've got a $wpdb->get_var call that's directly referencing 'wp_posts' - but 'wp_posts' will not exist if the user has specified a different table prefix on installation. Use $wpdb->prefix to get the defined prefix for the site instead.

That's the first thing that I noticed on installation; I didn't check for other instances, but anywhere you're using $wpdb, you should access the $wpdb->prefix instead of hardcoding 'wp_'.

#32 @joedolson
3 years ago

Regarding accessibility, before I noticed the database error, I had already noticed three issues you'll need to correct:

1) Headings: your headings hierarchy is not sequential; take a look at the accessibility ready guidelines for the specific criteria we're judging on. https://make.wordpress.org/themes/handbook/review/accessibility/required/

2) Keyboard accessibility: Your dropdown menus are not visibly accessible from the keyboard. The :hover behavior needs to be triggered when using the tab key to navigate through your site.

3) Color contrast: the tagline color and the post date color have a color contrast of 2.43:1, where a minimum contrast of 4.5:1 is required - you'll need to darken those colors. Your link color gives a contrast of 3.15:1, with the same 4.5:1 requirement.

4) Images: The 'Edit' link has no link text

5) Focus management: the lightbox opened by the 'Sharer' functionality does not move focus into the lightbox, so a keyboard user or screen reader user will have a lot of difficulty using that feature.

There are almost certainly others, but based on this 5 minute scan, you should review the accessibility guidelines and fix the above issues before I continue the review.

#33 @jeremyHixon
3 years ago

Thanks @djrmom and @joedolson,

I'm on it.

#34 @jeremyHixon
3 years ago

Excuse me @joedolson, but do you have any links to help with number 2 on your list? I'm not having too much luck with it and I'd rather get that remedied than drop the accessibility tag.

#35 @joedolson
3 years ago

Take a look at https://github.com/wpaccessibility/a11ythemepatterns/tree/master/dropdown-menus; that might help you. It's a couple of example of ways to make dropdown menus accessible.

#36 @jeremyHixon
2 years ago

@djrmom and @joedolson I should have this ready for another check in the next couple days. Thank you for your patience.

#37 @themetracbot
2 years ago

  • Summary changed from THEME: Rational Start – 1.0.1 to THEME: Rational Start – 1.0.2

#38 @jeremyHixon
2 years ago

@djrmom and @joedolson

Again, thanks for your patience.

  • Header hierarchy changed
  • Tab/focus navigation added to main nav
  • Focus added to sharer
  • Screen reader text to the edit button
  • Contrast increased

#39 @joedolson
2 years ago

1: Headings (fail)

The headings hierarchy still seems like a problem; the primary site title is an H3 - it should either not be a heading at all or be an H1. If it's an H1, the other titles should be an H2, so that the headings are sequential; the relationship between any H2 and it's parent H1 needs to be apparent.

2) ARIA Landmarks - pass

3) Link text - pass

4) Controls - Pass

5) Keyboard Accessibility - fail

I'm getting some odd behaviors in the keyboard navigation using the unit test data. On the "Pages" menu in the unit test data, using the "Testing" menu, I can't access the dropdown. On Firefox, it just skips it; and on Chrome, it sends keyboard focus in a loop. I've done a small amount of experimentation with changing menus around, and this seems to always happen on a dropdown on the first item in the menu. You'll need to take a look at that.

Also, the skip to content link should probably not have transparency; the collision with the site title is making it hard to read if there's no header image, which is always a possibility.

I also think it could be confusing to reach the toggle for opening sub menus *before* seeing what the parent link was. The content is ordered so that you first tab to "Toggle Subpages", and *then* toggle to the link which is the top level menu item. That would be much easier to use if they were in the other order. With this order, if somebody wants to get to a sub page under 'About', they'd tab past 'Toggle Subpages' until they got to About, then they'd need to reverse - or they might think that the next toggle was actually the one that they wanted. Either way, confusion ensues.

6) Contrasts - Fail

Mostly good, but links in the footer are coming in at 3.76:1, which is only a pass for large text. Those should definitely be brighter. Anywhere you have the combination of #246d9f and #000000.

7) Skip links - pass. (Just note that I think you should change the opacity.)

8) Forms - pass.

9) Images - pass.

10) Media - not observed.

11) Not allowed - pass.

It's really pretty close; but the headings need some hierarchy and some kind of odd bug in the keyboard navigation of the dropdown menu are the major issues.

#40 @jeremyHixon
2 years ago

Thanks @joedolson

I'm on it.

#41 @jeremyHixon
2 years ago

Hey @joedolson, quick question about #1:

Is my HTML5 type of coding the biggest issue? According to the spec, each major element, like articles, have their own sort of scope when it comes to headline elements. So, technically, when you open the site you can have the title of the site be an h1 and cascade down from there, but when you open an article you can start over with h1 and move down within it separately. If this is a problem with accessibility then I'll be glad to change it but I'd like to know for my own purposes going forward so that I can code accurately to the tag

#42 @joedolson
2 years ago

Yes, using the HTML5 headline hierarchy model is a major problem - essentially, it doesn't work. No user agent has actually implemented an algorithm to make any sense out of that hierarchy model, and as a result it's essentially useless. Definitely better not to attempt to use it.

Here's a good article about it: http://www.paciellogroup.com/blog/2013/10/html5-document-outline/

That article dates to 2013, but it's still true.

#43 @jeremyHixon
2 years ago

Thank you for taking the time to explain @joedolson, I'll get to work on correcting this as well as the rest of the problems.

#44 @themetracbot
2 years ago

  • Summary changed from THEME: Rational Start – 1.0.2 to THEME: Rational Start – 1.0.3

#45 @jeremyHixon
2 years ago

@joedolson, here are the updates:

  • Accessible navigation bug fixed
  • Footer contrast increased
  • Skip link made more prominent
  • Navigation controls on mobile re-ordered

#46 @djrmom
2 years ago

Hi @joedolson, Just checking on the accessibility review for this. Thanks :)

#47 @joedolson
2 years ago

I've been getting very irregular notifications on Theme Track tickets; thanks for re-pinging me. Will get on it ASAP.

#48 @joedolson
2 years ago

1) Headings: Fail

Still doesn't have a valid heading hierarchy; in testing the home page with posts, I couldn't find any H1 heading, so the H2 heading was skipping that hierarchical level. After getting through the H2s, it drops to h4, so that's also out of hierarchy.

The guidelines stipulate that you can't skip heading levels when going down the hierarchy (H1 > H2, H2 > H3, etc.), so both of these issues are problems.

5) Keyboard: Pass (ish)

Skip to content link currently falls underneath the Adminbar; this is relatively minor, but it would be good if it appeared above the admin bar (had a higher z-index).

6) Contrasts: Pass

#49 @djrmom
2 years ago

Hi @jeremyHixon,

Please let me know your progress on the lastest updates to keep this ticket open.

Thanks,
Jenny

#50 @jeremyHixon
2 years ago

Sorry, I must have missed the email notification from @joedolson's update. Thanks Mr. Dolson.

I'll make these changes tonight.

Thanks for checking in @djrmom.

#51 @themetracbot
2 years ago

  • Summary changed from THEME: Rational Start – 1.0.3 to THEME: Rational Start – 1.0.4

#52 @themetracbot
2 years ago

  • Summary changed from THEME: Rational Start – 1.0.4 to THEME: Rational Start – 1.0.5

#53 @jeremyHixon
2 years ago

Made some changes @joedolson:

Switched up the headline structure and I believe they all check out now.

Moved the "skip to content" link above the admin bar.

cc/@djrmom

#54 @joedolson
2 years ago

You're approved for accessibility-ready!

I did notice an issue you need to fix that isn't related to accessibility, however. In widgets.php / rational_child_pages, you've got a SQL statement to get post counts. It's got the table name hardcoded as wp_posts, which is only true if the user has not changed the table extension, and will otherwise throw a database error.

You'll need to get the table prefix from $wpdb->prefix

#55 @jeremyHixon
2 years ago

Ah, right, thanks for catching that @joedolson, I'll fix that really fast.

#56 @themetracbot
2 years ago

  • Summary changed from THEME: Rational Start – 1.0.5 to THEME: Rational Start – 1.0.6

#57 @joedolson
2 years ago

Awesome. Thanks for your hard work!

#58 @djrmom
2 years ago

  • Owner changed from jayantisolanki to djrmom
  • Status changed from reopened to reviewing

#59 @djrmom
2 years ago

  • Cc djrmom joedolson removed
  • Resolution set to live
  • Status changed from reviewing to closed

Thanks @jeremyHixon and @joedolson!

Note: See TracTickets for help on using tickets.