Tweaks following npm package updates for Beta 3 #5441

mikachan · 2023-10-09T18:08:25Z

Trac ticket: https://core.trac.wordpress.org/ticket/59411

This is a follow-up to #5439, as some review comments were missed just before commit.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Co-Authored-By: Felix Arntz <felix-arntz@leaves-and-love.net>

On Latest Posts block. Co-Authored-By: Felix Arntz <felix-arntz@leaves-and-love.net>

mikachan · 2023-10-09T18:18:20Z

src/wp-includes/blocks/latest-posts.php

+					$trimmed_excerpt .= '… <a href="' . esc_url( $post_link ) . '" rel="noopener noreferrer">';
 					$trimmed_excerpt .= sprintf(
-						/* translators: 1: A URL to a post, 2: The static string "Read more", 3: The post title only visible to screen readers. */
-						__( '… <a href="%1$s" rel="noopener noreferrer">%2$s<span class="screen-reader-text">: %3$s</span></a>' ),
-						esc_url( $post_link ),
-						__( 'Read more' ),
+						/* translators: %s: The post title only visible to screen readers */
+						__( 'Continue reading <span class="screen-reader-text">%s</span>' ),
 						esc_html( $title )
 					);
+					$trimmed_excerpt .= '</a>';


@swissspidy, pinging you here as these changes have moved from #5439 (comment) to here 🙇

Edit: Sorry, just saw your comment on the other PR! Copying over for completeness:

Rule of thumb is to avoid string concatenation like in this suggestion.
The ellipsis should be part of the translatable string, which means keeping the a tag in there, too. But ses, the „Read more“ should be in there as well and not added via a placeholder.

cc. @ramonjd @andrewserong for your thoughts here as you were involved in the original GB PR.

Again, no string concatenation please. The ellipsis AND the link tag should pe part of the translatable string.

Strings targeted for screen readers have this comment in core:
/* translators: Hidden accessibility text. */
And combined with a placeholder, something like this:
/* translators: Hidden accessibility text. %s: Post title */

Thanks @swissspidy! I've updated this in 6e74cd9 by removing the string concatenation and moving "Read more" into the translatable string. I think this is closer to your suggestions.

@kebbet, thanks for this, I've also updated the comment to this:

/* translators: 1: A URL to a post, 2: Hidden accessibility text: Post title */

Does this make sense?

Thanks for the feedback here, folks! And to @mikachan for updating the block 🙇🏻

Just to provide context, the original idea was to use the already-translated "Read more" - so thanks for the clarification on that approach.

I've got a PR going in Gutenberg to sync the changes from this patch. I'll give it a final update after this PR closes (should there be any further changes).

felixarntz

Thanks for opening the follow up PR @mikachan!

felixarntz · 2023-10-09T19:23:21Z

src/wp-includes/blocks/latest-posts.php

@@ -152,10 +152,9 @@ function render_block_core_latest_posts( $attributes ) {
 				if ( $excerpt_length <= $block_core_latest_posts_excerpt_length ) {
 					$trimmed_excerpt  = substr( $trimmed_excerpt, 0, -11 );
 					$trimmed_excerpt .= sprintf(
-						/* translators: 1: A URL to a post, 2: The static string "Read more", 3: The post title only visible to screen readers. */
-						__( '… <a href="%1$s" rel="noopener noreferrer">%2$s<span class="screen-reader-text">: %3$s</span></a>' ),
+						/* translators: The static string "Read more". %1$s: A URL to a post, %2$s: Hidden accessibility text: Post title */


There's some description in here that probably shouldn't be, I think these comments should only include the description of the placeholders.

Suggested change

/* translators: The static string "Read more". %1$s: A URL to a post, %2$s: Hidden accessibility text: Post title */

/* translators: 1: URL to a post, 2: post title */

Indeed!
I recommend checking the handbook for best practices like that

I added The static string "Read more" here as the "Read more" string is now part of this single placeholder. Is it OK if this isn't described in the comments, or is there a different way to do this?

Oh, I see from the examples here that any translatable text doesn't need to be described specifically, only the variables. Makes sense, the suggestion above works 🎉

Although as per @kebbet's suggestion, should we keep the "Hidden accessibility text" comment?

felixarntz · 2023-10-09T19:24:46Z

src/wp-includes/blocks/latest-posts.php

-						/* translators: 1: A URL to a post, 2: The static string "Read more", 3: The post title only visible to screen readers. */
-						__( '… <a href="%1$s" rel="noopener noreferrer">%2$s<span class="screen-reader-text">: %3$s</span></a>' ),
+						/* translators: The static string "Read more". %1$s: A URL to a post, %2$s: Hidden accessibility text: Post title */
+						__( '… <a href="%1$s" rel="noopener noreferrer">Read more<span class="screen-reader-text">: %2$s</span></a>' ),


Curious why we're using "Read more: {title}" here, as it reads a bit weird. Most existing core themes have been using something like "Continue reading {title}", which makes for a better reading flow.

Also see #5439 (comment)

I'm not sure why this wording has been chosen, so pinging @ramonjd @andrewserong to hopefully help here.

Thanks for the ping. "Read more" has been a feature of this block for a long time and I didn't want to change things up too much. So the basic reason was backwards compatibility.

That meant deciding how to structure the string so that it made sense grammatically. A semi-colon is fine.

The other option could have been "Read more of", or about, but I discarded the superfluous preposition.

I'm happy for the wording to change completely if folks think it best. As mentioned, my only intention was to avoid the situation where existing sites install 6.4, and are then left wondering why the "Read more" link text has changed.

P.S.: I'll make a note to backport any changes made here back to Gutenberg. Thanks a lot for helping to tidy this up, everyone!

Yeah the main reason the post title is added after the read more is so that when a screen reader provides a list of links on the page there is some context to each link and not just a bunch of "read more"s. In those circumstances something like "continue reading" won't make any more sense than "read more".

In any case this is probably not something to be changed during Beta as it's not, strictly speaking, a bug 😄

Removed var

tellthemachines

Based on the feedback on this and the previous PR, these changes LGTM!

tellthemachines · 2023-10-10T00:50:23Z

src/wp-includes/blocks/latest-posts.php

-						/* translators: 1: A URL to a post, 2: The static string "Read more", 3: The post title only visible to screen readers. */
-						__( '… <a href="%1$s" rel="noopener noreferrer">%2$s<span class="screen-reader-text">: %3$s</span></a>' ),
+						/* translators: The static string "Read more". %1$s: A URL to a post, %2$s: Hidden accessibility text: Post title */
+						__( '… <a href="%1$s" rel="noopener noreferrer">Read more<span class="screen-reader-text">: %2$s</span></a>' ),


Yeah the main reason the post title is added after the read more is so that when a screen reader provides a list of links on the page there is some context to each link and not just a bunch of "read more"s. In those circumstances something like "continue reading" won't make any more sense than "read more".

In any case this is probably not something to be changed during Beta as it's not, strictly speaking, a bug 😄

ramonjd · 2023-10-10T01:15:43Z

I've tested the latest comments and it LGTM.

I was wondering whether, instead of trimming the last 11 chars in the event of finding […], we could use wp_trim_words

				if ( $excerpt_length <= $block_core_latest_posts_excerpt_length ) {
					$excerpt_more = sprintf(
						/* translators: 1: A URL to a post, 2: Hidden accessibility text: Post title */
						__( '… <a href="%1$s" rel="noopener noreferrer">Read more<span class="screen-reader-text">: %2$s</span></a>' ),
						esc_url( $post_link ),
						esc_html( $title )
					);
					$trimmed_excerpt = wp_trim_words( $trimmed_excerpt, $excerpt_length, $excerpt_more );
				}

But looking into it, we're doing nearly the same thing , wp_trim_words just "trims" the ellipsis, treating it as a word - it can be explored in a follow up.

I will add that, while working on the translation, I found this block to be a little inconsistent in the way it presents trimmed excerpts in the editor and the frontend.

I think it's due to the priority (20) we're using when applying the filter.

Editor

Frontend

Using 99 seems to fix it:, e.g., add_filter( 'excerpt_length', 'block_core_latest_posts_get_excerpt_length', 99 );

Related issue: WordPress/gutenberg#33027

tellthemachines · 2023-10-10T03:07:06Z

Oh, I just realised these changes are to block-library files, so they need to be done in Gutenberg and followed by another package update. We shouldn't commit this changeset to core because it will be wiped the next time the packages are built 😅 I can put up a PR!

ramonjd · 2023-10-10T03:18:13Z

I can put up a PR!

I have one here if you wanna highjack it 👍🏻

WordPress/gutenberg#55181

tellthemachines · 2023-10-10T03:19:34Z

I have one here if you wanna highjack it 👍🏻

I just did WordPress/gutenberg#55184 lol

tellthemachines · 2023-10-10T03:26:50Z

Ok closed mine and reviewed yours @ramonjd !

Edit: I think we'd better close this PR to avoid confusion. I left a comment in the previous package update PR about the @return void changes; in any case they'll have to be done in Gutenberg too.

Removed var

* Site Editor Styles Screen: Fix dancing styles previews (#55183) * Pulling across changes from WordPress/wordpress-develop#5441 (#55181) Removed var * Add `aria-label` attribute to navigation block only when it is open (#54816) * Add `aria-label` only when is open * Remove unnecessary `label` property in edit * Escape translation * Move navigation context to `wp_json_encode` * Add `wp_json_encode` flags * Paste: fix MS Word list paste (#55127) * Paste: fix MS Word list paste * Match mso-list:Ignore * Fix inline paste * Fix scrollbars on pattern transforms (#55069) * Fix scrollbars on pattern transforms * Fix single pattern previews * Improve classname semantics * Remove modal title * Reset styles on window resize (#55077) Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> --------- Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Mario Santos <34552881+SantosGuillamot@users.noreply.github.com> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com>

mikachan · 2023-10-10T11:52:02Z

Thanks everyone for your help and feedback here! 🚀

mikachan and others added 2 commits October 9, 2023 19:03

Remove @return void comments

1f2451f

Co-Authored-By: Felix Arntz <felix-arntz@leaves-and-love.net>

Refactor read more link translation

ca48382

On Latest Posts block. Co-Authored-By: Felix Arntz <felix-arntz@leaves-and-love.net>

mikachan commented Oct 9, 2023

View reviewed changes

mikachan added 3 commits October 9, 2023 20:00

Remove additional string concatenation

6e74cd9

Update hidden accessibility text comment

ad845b6

Add translator text for read more string

fdfbc77

felixarntz reviewed Oct 9, 2023

View reviewed changes

felixarntz requested a review from swissspidy October 9, 2023 19:26

Remove Read more description

199ea1e

ramonjd mentioned this pull request Oct 10, 2023

Latest Posts: sync updates from Core (6.4) WordPress/gutenberg#55181

Merged

ramonjd added a commit to WordPress/gutenberg that referenced this pull request Oct 10, 2023

Pulling across changes from WordPress/wordpress-develop#5441

e7fe2f3

Removed var

tellthemachines approved these changes Oct 10, 2023

View reviewed changes

ramonjd mentioned this pull request Oct 10, 2023

Latest Posts block: Check priority of excerpt_length filter WordPress/gutenberg#33027

Open

tellthemachines mentioned this pull request Oct 10, 2023

Update translatable string for Latest Posts block WordPress/gutenberg#55184

Closed

tellthemachines closed this Oct 10, 2023

ramonjd added a commit to WordPress/gutenberg that referenced this pull request Oct 10, 2023

Pulling across changes from WordPress/wordpress-develop#5441 (#55181)

2d86e90

Removed var

mikachan deleted the updates-post-gb-sync-beta3 branch October 10, 2023 09:18

mikachan pushed a commit to WordPress/gutenberg that referenced this pull request Oct 10, 2023

Pulling across changes from WordPress/wordpress-develop#5441 (#55181)

afed927

Removed var

mikachan mentioned this pull request Oct 10, 2023

Update npm packages to latest versions for 6.4 Beta 3 v2 #5449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweaks following npm package updates for Beta 3 #5441

Tweaks following npm package updates for Beta 3 #5441

mikachan commented Oct 9, 2023

mikachan Oct 9, 2023 •

edited

Loading

mikachan Oct 9, 2023

swissspidy Oct 9, 2023

kebbet Oct 9, 2023

mikachan Oct 9, 2023 •

edited

Loading

ramonjd Oct 10, 2023

felixarntz left a comment

felixarntz Oct 9, 2023

swissspidy Oct 9, 2023

mikachan Oct 9, 2023

mikachan Oct 9, 2023

felixarntz Oct 9, 2023

mikachan Oct 9, 2023

ramonjd Oct 9, 2023 •

edited

Loading

tellthemachines Oct 10, 2023

tellthemachines left a comment

tellthemachines Oct 10, 2023

ramonjd commented Oct 10, 2023 •

edited

Loading

tellthemachines commented Oct 10, 2023

ramonjd commented Oct 10, 2023

tellthemachines commented Oct 10, 2023

tellthemachines commented Oct 10, 2023 •

edited

Loading

mikachan commented Oct 10, 2023

	/* translators: The static string "Read more". %1$s: A URL to a post, %2$s: Hidden accessibility text: Post title */
	/* translators: 1: URL to a post, 2: post title */

Tweaks following npm package updates for Beta 3 #5441

Tweaks following npm package updates for Beta 3 #5441

Conversation

mikachan commented Oct 9, 2023

mikachan Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikachan Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellthemachines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd commented Oct 10, 2023 • edited Loading

Editor

Frontend

tellthemachines commented Oct 10, 2023

ramonjd commented Oct 10, 2023

tellthemachines commented Oct 10, 2023

tellthemachines commented Oct 10, 2023 • edited Loading

mikachan commented Oct 10, 2023

mikachan Oct 9, 2023 •

edited

Loading

mikachan Oct 9, 2023 •

edited

Loading

ramonjd Oct 9, 2023 •

edited

Loading

ramonjd commented Oct 10, 2023 •

edited

Loading

tellthemachines commented Oct 10, 2023 •

edited

Loading