Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

comments_popup_link defaults for accessibility #907

Closed
samikeijonen opened this issue Jan 26, 2016 · 13 comments
Closed

comments_popup_link defaults for accessibility #907

samikeijonen opened this issue Jan 26, 2016 · 13 comments

Comments

@samikeijonen
Copy link
Contributor

I was wondering how _s uses comments_popup_link function:

https://github.com/Automattic/_s/blob/master/inc/template-tags.php#L64

comments_popup_link( esc_html__( 'Leave a comment', '_s' ), esc_html__( '1 Comment', '_s' ), esc_html__( '% Comments', '_s' ) );

Core take care of this a little better by default what it comes for accessibility. For example Core uses for 1 Comment string like this:

sprintf( __( '1 Comment<span class="screen-reader-text"> on %s</span>' ), $title )

Would it be better to use defaults for this one?

@dmtrmrv
Copy link
Contributor

dmtrmrv commented Jan 26, 2016

I believe it is done this way because it is generally not a good thing to let translators handle the markup. But I agree that adding the .screen-reader-text class would benefit the accessibility.

@samikeijonen
Copy link
Contributor Author

I translate Core and it's not a problem:) But yes, in general it's not the best option to leave html markup for translators.

Actually Core defaults are better for two reasons:

  1. Better accessibility.
  2. Fewer strings to translate in your theme.

@dmtrmrv
Copy link
Contributor

dmtrmrv commented Jan 26, 2016

Agree, but what I'm saying is, we can still go the accessibility road, without markup in translatable strings:

sprintf( __( '1 Comment on %s' ), '<span class="screen-reader-text">' . $title . '</span>' )

@samikeijonen
Copy link
Contributor Author

That would look weird on front end for non screen reader users. 1 Comment on and then nothing.

@dmtrmrv
Copy link
Contributor

dmtrmrv commented Jan 26, 2016

Silly me, didn't think of that :)

@obenland
Copy link
Member

We could probably change the first argument to 'Leave a Comment<span class="screen-reader-text"> on %s</span>' and lose the rest.

@samikeijonen
Copy link
Contributor Author

That would be step forward. I'd even leave Core handle the first argument, but that's just me:)

@obenland
Copy link
Member

I thought about that too, but it is more encouraging than the default wording.

@samikeijonen
Copy link
Contributor Author

Do you want me to create pull request or what are we waiting for:)

@karmatosed
Copy link
Contributor

@samikeijonen if you can do a PR that would be great. I'd suggest what @obenland did to have more encouraging wording.

@samikeijonen
Copy link
Contributor Author

Ok, I'll send a PR.

@samikeijonen
Copy link
Contributor Author

I used wp_kses, hope that's ok.

@sixhours
Copy link
Contributor

Looks like this was merged in #915, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants