WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 22 months ago

#31979 closed enhancement (wontfix)

Twenty Thirteen: Improving i18n for "Continue reading"

Reported by: dregad Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.1.1
Component: Bundled Theme Keywords: has-patch close
Focuses: Cc:

Description

When using French translation of 2013 theme, the "More" link is translated from "Continue reading title" to "Continuer la lecture de title".

This is fine, except for the fact that title is wrapped in a span with screen-reader-text class that causes it to be hidden, leading to display of just "Continuer la lecture de" which is an incorrect translation - it should be "Continuer la lecture".

I would like to suggest the following code change in content.php

--- content.php.orig    2015-04-15 15:34:44.186854600 +0200
+++ content.php.new     2015-04-15 15:41:09.645854600 +0200
@@ -41,8 +41,8 @@
                <?php
                        /* translators: %s: Name of current post */
                        the_content( sprintf(
-                               __( 'Continue reading %s <span class="meta-nav">&rarr;</span>', 'twentythirteen' ),
-                               the_title( '<span class="screen-reader-text">', '</span>', false )
+                               __( 'Continue reading<span class="screen-reader-text"> %s</span>&nbsp;<span class="meta-nav">&rarr;</span>', 'twentythirteen' ),
+                               get_the_title()
                        ) );

                        wp_link_pages( array( 'before' => '<div class="page-links"><span class="page-links-title">' . __( 'Pages:', 'twentythirteen' ) . '</span>', 'after' => '</div>', 'link_before' => '<span>', 'link_after' => '</span>' ) );

This would allow moving "de" within the span for the French translation (and maybe others languages too):

'Continuer la lecture<span class="screen-reader-text"> de %s</span>&nbsp;<span class="meta-nav">&rarr;</span>'

I am not sure whether it is correct to put the class name within the translation string. If not, maybe someone more qualified than I am can come up with a better solution for the problem.

Attachments (1)

31979.diff (2.2 KB) - added by atimmer 2 years ago.

Download all attachments as: .zip

Change History (18)

#1 @SergeyBiryukov
2 years ago

  • Component changed from Themes to Bundled Theme
  • Summary changed from Improving i18n for "Continue reading" in twenty thirteen theme to Twenty Thirteen: Improving i18n for "Continue reading"

#2 follow-ups: @SergeyBiryukov
2 years ago

Introduced in [30390] (#30178).

This would also apply to Twenty Fourteen and Fifteen, see [30389] (#30176) and [30237] (#30135).

I guess we need one more sprintf() here:

the_content( sprintf(
	/* translators: %s: Name of current post */
	__( 'Continue reading %s <span class="meta-nav">&rarr;</span>', 'twentythirteen' ),
	/* translators: %s: Name of current post */
	sprintf( _x( '%s', 'post name' ), the_title( '<span class="screen-reader-text">', '</span>', false ) )
) );

So that '%s' could be translated as 'de %s'.

#3 in reply to: ↑ 2 ; follow-up: @dregad
2 years ago

Hi Sergey, and thanks for your response.

Replying to SergeyBiryukov:

	sprintf( _x( '%s', 'post name' ), the_title( '<span class="screen-reader-text">', '</span>', false ) )

So that '%s' could be translated as 'de %s'.

Is post name enough context for translators to understand what this is about ?

#4 in reply to: ↑ 3 @SergeyBiryukov
2 years ago

Replying to dregad:

Is post name enough context for translators to understand what this is about ?

The context is just to separate out the string in case there's another '%s' string in different context somewhere in the theme (currently there isn't). But yeah, it can also be 'name of current post'.

The %s placeholder should also be explained in translator comment above the string.

#5 @lancewillett
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Thanks for the ticket -- we're in freeze for 4.2 but can look at it right away for the next release of the bundled themes (summer 2015).

#6 @dregad
2 years ago

Hi Lance,

Thanks for your feedback and for taking this into consideration. Summer 2015 is good enough, I applied a quick&dirty patch to my child theme for now so there's no rush for me.

#7 @lancewillett
2 years ago

  • Milestone changed from Future Release to 4.3

Needs a patch.

#8 @obenland
2 years ago

Why can't it be translated with 'Continuer la lecture<span class="screen-reader-text"> de %s</span>&nbsp;<span class="meta-nav">&rarr;</span>' now?

Or 'Continuer la lecture<span class="screen-reader-text"> de </span>%s&nbsp;<span class="meta-nav">&rarr;</span>' if you want.

@atimmer
2 years ago

#9 in reply to: ↑ 2 @atimmer
2 years ago

  • Keywords has-patch added; needs-patch removed

Replying to SergeyBiryukov:

I guess we need one more sprintf() here:

the_content( sprintf(
	/* translators: %s: Name of current post */
	__( 'Continue reading %s <span class="meta-nav">&rarr;</span>', 'twentythirteen' ),
	/* translators: %s: Name of current post */
	sprintf( _x( '%s', 'post name' ), the_title( '<span class="screen-reader-text">', '</span>', false ) )
) );

This solution doesn't solve the problem raised in the ticket, de would still be outside of the screen-reader-text span.

31979.diff is simply the patch proposed in the OP applied to all three themes.

#10 @obenland
2 years ago

  • Keywords close added

I really don't think we need to change anything in the translation string in Twenty Thirteen. HTML tags don't get stripped, so the french translation can just add screen reader text spans as needed, like I mentioned in my comment above.

#11 @obenland
2 years ago

  • Milestone 4.3 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

@dregad: There shouldn't be any code change necessary, you can use the following translation to make it work in French: 'Continuer la lecture<span class="screen-reader-text"> de %s</span>&nbsp;<span class="meta-nav">&rarr;</span>'.

#12 @dregad
2 years ago

@obenland thanks for the feedback. I'm fine with your approach of fixing this through translation instead of code.

I can make the necessary changes locally, no problem, but aren't you going to fix this translation globally and release an update to the plugin ?

#13 @dregad
2 years ago

I confirm that @obenland's suggestion fixes the display issue I originally reported.

However, I wanted to bring to your attention that the HTML generated after updating the translation now has 2 "screen-reader-text" <span> tags, one within the other as shown below

<a href="http://example.com/2015/06/article/#more-860" class="more-link">
  Continuer la lecture
  <span class="screen-reader-text"> de 
     <span class="screen-reader-text">Titre de l'article</span>
  </span>
  &nbsp;<span class="meta-nav">→</span>
</a>

I am not sure whether that would have any impact on processing by a screen reader.

#14 @obenland
2 years ago

You could also translate it with 'Continuer la lecture<span class="screen-reader-text"> de </span>%s&nbsp;<span class="meta-nav">&rarr;</span>' to avoid that.

This ticket was mentioned in Slack in #polyglots by dregad. View the logs.


2 years ago

#16 follow-up: @xibe
2 years ago

Hey there. As mentioned on Slack, I validated the string changes (even though GlotPress screamed murder, saying "Too many tags in translation").

In truth, I'm not too keen on having more code in the translation than there is in the original string (as does GlotPress, hence its warning). I am surprised the recommendation would be to do that. I do hope this ticket end up in a fix landing in the original strings.

#17 in reply to: ↑ 16 @SergeyBiryukov
22 months ago

Replying to xibe:

In truth, I'm not too keen on having more code in the translation than there is in the original string (as does GlotPress, hence its warning). I am surprised the recommendation would be to do that. I do hope this ticket end up in a fix landing in the original strings.

The suggested fix is now implemented in Twenty Sixteen: https://github.com/WordPress/twentysixteen/pull/383.

Note: See TracTickets for help on using tickets.