WordPress.org

Make WordPress Themes

Opened 18 months ago

Closed 12 months ago

Last modified 9 months ago

#34513 closed theme (live)

THEME: Octothorpe – 1.1.4

Reported by: henry.wright Owned by: acosmin
Priority: new theme Keywords: theme-octothorpe accessibility-ready
Cc: henrywright@…

Change History (47)

#1 @themetracbot
17 months ago

  • Summary changed from THEME: Octothorpe – 1.0.0 to THEME: Octothorpe – 1.1.0

#2 @acosmin
13 months ago

  • Keywords changed from theme-octothorpe, accessibility-ready to theme-octothorpe accessibility-ready
  • Owner set to acosmin
  • Status changed from new to reviewing

#3 follow-up: @acosmin
13 months ago

Theme Check - ok

Required

  1. functions.php
    • l9-25, why are you re-registering Core widgets and removing that action, it's not allowed, please remove those lines.
    • remove theme support for:
      • l52 search-form - you're using a custom template in searchform.php instead.
      • l53 comment-list- because of octothorpe_comment()
      • l54 comment-form - because of comment_form_defaults()
    • l79 - use this method to include Google fonts link
    • l87 change octothorpe to octothorpe-style
    • remove l92-94, not allowed. It's lost on theme change.


  1. searchform.php
    • get_search_query() doesn't need escaping
  1. in \template-parts\ use Core functions instead of calling $wp_query
  1. Your theme doesn't look at all like the screenshot
    • white background
    • no post's excerpts
    • it should display your homepage as it actually looks
  1. You should remove the accessibility-ready tag, I don't think it will pass. Or you can wait, but it will take some time.

#4 @henry.wright
13 months ago

Thanks for reviewing @acosmin

I'll begin making those changes.

#5 @rabmalin
13 months ago

it should display your homepage as it actually looks

It is not necessary to be post listings. It is OK to display a page in the screenshot.

#6 @acosmin
12 months ago

We're going to need an update on this, we're near the 7 days deadline. Thank you!

#7 @henry.wright
12 months ago

I'm in the process of making the changes you requested. I'll commit as soon as I've finished. Thanks

#8 @henry.wright
12 months ago

@acosmin I have a few queries.

Your theme doesn't look at all like the screenshot

The screenshot is of the sample page included by WordPress. Should my screenshot instead be of the page_for_posts?

You should remove the accessibility-ready tag, I don't think it will pass. Or you can wait, but it will take some time.

I'd like my theme to be reviewed for accessibility. I'm happy to make changes where needed. Can you kick start an accessibility review? The feedback will be appreciated.

#9 @acosmin
12 months ago

I am not an accessibility reviewer, someone else will do that review and it will take a few weeks before it starts.

Anyway, please submit an update because in 3 days I will need close this ticket (2 weeks since my first comment and you didn't submit 1 line of code).

#10 @henry.wright
12 months ago

Anyway, please submit an update because in 3 days I will need close this ticket (2 weeks since my first comment and you didn't submit 1 line of code).

Please bear with me. I will post within your 3 day timeframe.

#11 @sami.keijonen
12 months ago

Hi. I'll be doing accessibility review. But I'll wait until the initial review is done.

If you already know fixes for accessibility feel free to add them in next version also. Thanks!

#12 @henry.wright
12 months ago

Hi @sami.keijonen

Thanks, I'm keen to make the theme accessible so your feedback will be much appreciated. I'll be pushing a new version today or tomorrow

#13 in reply to: ↑ 3 @henry.wright
12 months ago

Thank you again @acosmin for your feedback. Everything has now been done aside from the following:

  • I haven't removed theme support for comment-list and comment-form
  • Usage of $wp_query in template-parts/ is necessary because I'm conditionally outputting a <nav> element
  • My theme does look like the screenshot if you view the sample page provided by WordPress
  • I haven't removed the accessibility-ready tag

Please let me know if this is acceptable.

Last edited 12 months ago by henry.wright (previous) (diff)

#14 @themetracbot
12 months ago

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

Octothorpe - 1.1.1

Octothorpe is a theme for blogging.

Theme URL - https://github.com/lsquo/octothorpe
Author URL - https://lsquo.com

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

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

History:

Ticket Summary Status Resolution Owner
#34513 THEME: Octothorpe – 1.1.4 closed live acosmin

(this ticket)

#39275 THEME: Octothorpe – 1.1.5 closed live themetracbot


https://themes.svn.wordpress.org/octothorpe/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: 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.
  • RECOMMENDED: No reference to add_editor_style() was found in the theme. It is recommended that the theme implement editor styling, so as to make the editor content match the resulting post output in the theme, for a better user experience.

#15 @acosmin
12 months ago

These issues were not fixed:

  1. remove theme support for...
  1. in \template-parts\ use Core functions instead of calling $wp_query

#16 @henry.wright
12 months ago

the functions I gave you as examples do exactly that.

No, they don't. The get_the_posts_pagination() function returns pagination like "1 2 3 4 Next".

I want to use "Previous Next" links only (no numbering) because I want to be consistent with the pagination links elsewhere in my theme. If a core function is available that will return "Previous Next" pagination links please let me know. Until then $wp_query is the only option here.

#17 @acosmin
12 months ago

I have you two examples for that type of navigation, the other one is the_posts_navigation() and does what you want.

#18 @henry.wright
12 months ago

the_posts_navigation() doesn't do what I need. It's a wrapper for echo get_the_posts_navigation().

If I use the_posts_navigation() and there's no page 2, I'll be left with an empty <nav></nav> in my HTML source because the function will output nothing.

<nav>
    <?php the_posts_navigation(); ?>
</nav>

Do you now see why I must use $wp_query? There simply isn't a core function available to do what I need. If there was one, I would use it.

#19 @acosmin
12 months ago

uhhhhhhhh. the_posts_navigation(); has that inside it and it will not display it if no pages are found. https://developer.wordpress.org/reference/functions/_navigation_markup/#source

You don't need the <nav> elements.

Would you like a new reviewer?

#20 @henry.wright
12 months ago

Would you like a new reviewer?

Why do I need a new reviewer?

#21 @acosmin
12 months ago

If you feel that I don't provide a proper review, I can ask to be replaced.

#22 @sami.keijonen
12 months ago

As @acosmin mentioned use the_posts_navigation() function, it has <nav> markup already build in.

#23 @henry.wright
12 months ago

@acosmin

If you feel that I don't provide a proper review, I can ask to be replaced.

Your review and feedback has been super helpful. I don't want to replace you.

@sami.keijonen

As I mentioned earlier, the_posts_navigation() doesn't output what I need. The function will output numbered pagination like this:

1 2 3 4 Next

I am using worded pagination in my theme. For example:

Previous Next

I do not wish to use numbered pagination in this instance because it will break consistency.

Hopefully that explains why $wp_query is needed?

#24 @sami.keijonen
12 months ago

The function will output numbered pagination like this:

It does not.

the_posts_pagination function output numbers like 1 2 3 4 Next.

#25 @henry.wright
12 months ago

My apologies, I see you referred to the_posts_navigation() and not the_posts_pagination().

There was a reason why I didn't use the_posts_navigation(). I don't want to output <h2 class="screen-reader-text">Text</h2>, which appears to be mandatory when using the_posts_navigation().

Last edited 12 months ago by henry.wright (previous) (diff)

#26 @sami.keijonen
12 months ago

I don't want to output <h2 class="screen-reader-text">Text</h2>

Don't worry about it. You need to have class screen-reader-text anyway in your style.css which means title is not visible on screen. And title is good for accessibility.

#27 @acosmin
12 months ago

or you can just:

function textdomain_remove_nav_h2() {
        return $template = '
        <nav class="navigation %1$s" role="navigation">
                <div class="nav-links">%3$s</div>
        </nav>';
}
add_filter( 'navigation_markup_template', 'textdomain_remove_nav_h2' );

#28 @henry.wright
12 months ago

Thanks @acosmin, I didn't know about the navigation_markup_template filter.

I'm away from the desk now for the weekend but will update first thing Monday.

#29 @henry.wright
12 months ago

@sami.keijonen picking up on your point about screen-reader-text, I notice Twenty Seventeen uses the deprecated clip CSS property to help hide it. clip-path is recommended instead but has poor browser support

Last edited 12 months ago by henry.wright (previous) (diff)

#30 @sami.keijonen
12 months ago

Twenty Seventeen have the current best approach for screen-reader-text class considering all browsers and screen readers.

But it's not required to use it for accessibility-ready tag.

#31 @henry.wright
12 months ago

Thanks for the link. I read that section of the handbook before I submitted for review but I'll have another read before I submit my next update.

Last edited 12 months ago by henry.wright (previous) (diff)

#32 @henry.wright
12 months ago

Apologies for the delay in publishing my update. I'll be doing it this evening.

#33 @themetracbot
12 months ago

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

Octothorpe - 1.1.2

Octothorpe is a theme for blogging.

Theme URL - https://github.com/lsquo/octothorpe
Author URL - https://lsquo.com

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

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

History:

Ticket Summary Status Resolution Owner
#34513 THEME: Octothorpe – 1.1.4 closed live acosmin

(this ticket)

#39275 THEME: Octothorpe – 1.1.5 closed live themetracbot


https://themes.svn.wordpress.org/octothorpe/1.1.2/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: 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.
  • RECOMMENDED: No reference to add_editor_style() was found in the theme. It is recommended that the theme implement editor styling, so as to make the editor content match the resulting post output in the theme, for a better user experience.

#34 @acosmin
12 months ago

@sami.keijonen you can continue with the accessibility review.

@henry.wright I will not be available until the 28-29th, so it may take a while for this theme to go live.

Thank you! Happy Holidays!

#35 @sami.keijonen
12 months ago

Thanks @acosmin.

I'm also leaving for a holiday in couple of hours:)

I'll try to check this end of 2016 or early 2017.

#36 @henry.wright
12 months ago

Thanks @acosmin and @sami.keijonen! Happy holidays to you both

#37 @sami.keijonen
12 months ago

Here is initial accessibility review based on the guidelines.

Keyboard Navigation

  • On input elements (like comment form fields) there is no focus styles when using firefox or IE.

Controls

Pass

Skip Links

  • Even though theme doesn't have much to skip in the header, skip to content link should be first thing after body. For example child themes could add navigation in the header.

Forms

  • Search form doesn't have a button or <input type="submit">.
  • Form works with the Enter key but submit button needs to be for present for screen readers. Submit button can be visually hidden for example using .screen-reader-class.

Headings

  • Pass

ARIA Landmark Roles

  • Aria landmark roles are missing in structural regions of a theme.

Link Text

  • Pass

Contrasts

  • .sticky time have insufficient color contrast.

Images

  • Pass

Media

  • Pass

Screen Reader Text

  • Pass
  • Noting that theme doesn't use .screen-reader-text class

#38 @henry.wright
12 months ago

Thanks @sami.keijonen for the accessibility review. I'll begin implementing the necessary changes and upload the new version once finished.

#39 @themetracbot
12 months ago

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

Octothorpe - 1.1.3

Octothorpe is a theme for blogging.

Theme URL - https://github.com/lsquo/octothorpe
Author URL - https://lsquo.com

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=octothorpe/1.1.2&new_path=octothorpe/1.1.3

History:

Ticket Summary Status Resolution Owner
#34513 THEME: Octothorpe – 1.1.4 closed live acosmin

(this ticket)

#39275 THEME: Octothorpe – 1.1.5 closed live themetracbot


https://themes.svn.wordpress.org/octothorpe/1.1.3/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: 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.
  • RECOMMENDED: No reference to add_editor_style() was found in the theme. It is recommended that the theme implement editor styling, so as to make the editor content match the resulting post output in the theme, for a better user experience.

#40 @sami.keijonen
12 months ago

Here is second round test. There are still couple of issues but getting there.

Keyboard Navigation

Pass

Skip Links

  • Skip link needs to be visible when keyboard focus moves to the link.

Forms

Pass

ARIA Landmark Roles

  • For consistency there should also be role="main" also on archive, home and search pages.
  • Nav in the footer should have <nav> markup with role="navigation".

Contrasts

Pass

#41 @themetracbot
12 months ago

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

Octothorpe - 1.1.4

Octothorpe is a theme for blogging.

Theme URL - https://github.com/lsquo/octothorpe
Author URL - https://lsquo.com

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=octothorpe/1.1.3&new_path=octothorpe/1.1.4

History:

Ticket Summary Status Resolution Owner
#34513 THEME: Octothorpe – 1.1.4 closed live acosmin

(this ticket)

#39275 THEME: Octothorpe – 1.1.5 closed live themetracbot


https://themes.svn.wordpress.org/octothorpe/1.1.4/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: 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.
  • RECOMMENDED: No reference to add_editor_style() was found in the theme. It is recommended that the theme implement editor styling, so as to make the editor content match the resulting post output in the theme, for a better user experience.

#42 @henry.wright
12 months ago

Thanks for the second round test, @sami.keijonen. I've uploaded 1.1.4 which should address the issues you found.

#43 @sami.keijonen
12 months ago

As far as I can tell, theme now passes a11y check, congrats!

#44 @henry.wright
12 months ago

That's great to hear! Thanks again

#45 @acosmin
12 months ago

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

@henry.wright congrats! Your theme should be live shortly!

#46 @henry.wright
12 months ago

That's brilliant, thanks @acosmin!

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


9 months ago

Note: See TracTickets for help on using tickets.