Comments Title: Make non-editable #40817
Conversation
ockham
added
[Type] Bug
|
Size Change: +4.63 kB (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
ockham
requested review from
ajitbohra,
ryanwelcher and
juanmaguitar
as code owners
| /* translators: %s: Post title. */ | ||
| placeholder = sprintf( __( 'One response to %s' ), postTitle ); |
There was a problem hiding this comment.
| /* translators: 1: Number of comments, 2: Post title. */ | ||
| _n( | ||
| '%1$s response to %2$s', | ||
| '%1$s responses to %2$s', | ||
| commentsCount | ||
| ), |
There was a problem hiding this comment.
| ); | ||
| } | ||
| } else if ( commentsCount === 1 ) { | ||
| placeholder = __( 'One response' ); |
There was a problem hiding this comment.
String exists in Core (per WP 6.0 Beta 4): https://github.com/WordPress/wordpress-develop/blob/04e9728701b3dde0260c7cdebfd726edbe62ac97/src/wp-includes/blocks/comments-title.php#L38
| placeholder = sprintf( __( 'Responses to %s' ), postTitle ); | ||
| } | ||
| } else if ( commentsCount === 1 ) { | ||
| placeholder = __( 'Response' ); |
There was a problem hiding this comment.
String exists in Core (per WP 6.0 Beta 4): https://github.com/WordPress/wordpress-develop/blob/04e9728701b3dde0260c7cdebfd726edbe62ac97/src/wp-includes/blocks/comments-title.php#L36
| } else if ( commentsCount === 1 ) { | ||
| placeholder = __( 'Response' ); | ||
| } else { | ||
| placeholder = __( 'Responses' ); |
There was a problem hiding this comment.
String exists in Core (per WP 6.0 Beta 3): https://github.com/WordPress/wordpress-develop/blame/04e9728701b3dde0260c7cdebfd726edbe62ac97/src/wp-includes/blocks/comments-title.php#L42
| } else { | ||
| placeholder = sprintf( | ||
| /* translators: %s: Number of comments. */ | ||
| _n( '%s responses', '%s responses', commentsCount ), |
There was a problem hiding this comment.
String doesn't exist in Core yet. However, this is arguably needed to fix the issue at the core of this PR (use sprintf with placeholders).
| } else if ( showPostTitle ) { | ||
| if ( commentsCount === 1 ) { | ||
| /* translators: %s: Post title. */ | ||
| placeholder = sprintf( __( 'Response to %s' ), postTitle ); |
There was a problem hiding this comment.
String doesn't exist in Core yet. However, this is arguably needed to fix the issue at the core of this PR (use sprintf with placeholders).
| '%1$s responses to %2$s', | ||
| $comments_count | ||
| ), | ||
| $comments_count, |
There was a problem hiding this comment.
| $comments_count, | |
| number_format_i18n( $comments_count ), |
| $comments_title = sprintf( | ||
| /* translators: %s: Number of comments. */ | ||
| _n( '%s responses', '%s responses', $comments_count ), | ||
| $comments_count |
There was a problem hiding this comment.
| $comments_count | |
| number_format_i18n( $comments_count ) |
| @@ -22,8 +22,8 @@ function render_block_core_comments_title( $attributes ) { | |||
| $show_post_title = ! empty( $attributes['showPostTitle'] ) && $attributes['showPostTitle']; | |||
| $show_comments_count = ! empty( $attributes['showCommentsCount'] ) && $attributes['showCommentsCount']; | |||
| $wrapper_attributes = get_block_wrapper_attributes( array( 'class' => $align_class_name ) ); | |||
| $post_title = $show_post_title ? sprintf( '"%1$s"', get_the_title() ) : null; | |||
| $comments_count = number_format_i18n( get_comments_number() ); | |||
There was a problem hiding this comment.
| $comments_count = number_format_i18n( get_comments_number() ); | |
| $comments_count = get_comments_number(); |
For the conditions and for the _n() calls this needs to be in integer form to work. In Japanese this will get translated to 一 and have unexpected results below.
There was a problem hiding this comment.
I took the code from WordPress Core, and it seems that get_comments_number returns a string always.
In Core, is not failing due to using == (Equal instead of Identical) on both translate_plural() L154 and select_plural_form() L122
There was a problem hiding this comment.
FWIW, the docs say
Return
(string|int) If the post exists, a numeric string representing the number of comments the post has, otherwise 0.
There was a problem hiding this comment.
Posting this Slack message by @peterwilsoncc:
My main concern was the translation early, as the number will get converted outside the 0-9 character set in some languages.
I thought I was seeing an int when testing but that have been a numeric string.
I overall agree that using an i18n'd number (which is turned into a string that might contain non-numeric characters) seems problematic for _n() calls 🤔 Maybe we can err on the side of caution and do something similar as the theme-compat code in Core that @c4rl0sbr4v0 mentioned:
- Use
number_format_i18n( get_comments_number() )only to substitutesprintf()placeholders - Use
get_comments_number()as argument in_n(), and in conditions.
We can continue to use the $comments_count variable for one of those. (I'd lean towards number_format_i18n( get_comments_number() ) since it's longer.)
There was a problem hiding this comment.
Or maybe it's safest to drop the $comments_count variable altogether to make the difference a bit more explicit.
There was a problem hiding this comment.
I updated the code, putting the number format only on the sprint, but keeping $comments_count as a variable for comparisons and _n function.
| $single_default_comment_label = $show_post_title ? __( 'Response to' ) : __( 'Response' ); | ||
| if ( $show_comments_count ) { | ||
| $single_default_comment_label = $show_post_title ? __( 'One response to' ) : __( 'One response' ); | ||
| if ( $show_comments_count && ! empty( $comments_count ) ) { |
There was a problem hiding this comment.
The effect of the empty() is that if the comment count is enabled, zero comments will result in Responses or Responses on Post Title. Due to the try... catch in the JavaScript I suspect this could display differently in the editor.
Is that the intent?
There was a problem hiding this comment.
Maybe the empty() was added because it's kind of a counterpart to the commentsCount !== undefined criterion on R110? cc/ @c4rl0sbr4v0
FWIW, I added that there in order to avoid an undefined responses to "Post Title" flash while loading. (This is only relevant to the Post Editor, where we're fetching the actual comments on the post, whereas in the Site Editor, we're using 3 hard-wired placeholders.)
Anyway, on the frontend, we bail early if there are no comments.
So I think we can safely remove the empty() criterion here
| if ( $show_comments_count && ! empty( $comments_count ) ) { | |
| if ( $show_comments_count ) { |
(We might need to make sure we're using the correct variable type on R110 if we end up changing $comments_count's definition, see.)
gziolo
added
the
Internationalization (i18n)
|
Changes LGTM, but since I'm the PR author, I can't give formal approval. @peterwilsoncc Would ya mind? 🙏 😊 |
adamziel
added
the
Backport to WP 6.6 Beta/RC
|
I cherry-picked this PR for WordPress 6.0 RC2 with 2560ee0. |
gziolo
removed
the
Backport to WP 6.6 Beta/RC
* Comments Title: Make non-editable * Avoid showing 'undefined' while loading * Update php to handle conditionals * Rename comment title variable * Add initial deprecation * Add supports deprecation * Add integration test for deprecation * Remove not needed check * Number format only on sprintf Co-authored-by: Carlos Bravo <[email protected]>
|
I came here to find out why the comments title isn't editable. I get it, but I think the hard-coded HTML nevertheless has a problem. Double quotation marks are hard-coded as part of the non-editable title, which is the norm in American English (AmE). However, in other varieties of English and in other languages, the quotation marks may be different. For example, in British English (BrE), single quotes are typically used (with some exceptions). Is it possible to change the quotation marks used for For example, instead of outputting this HTML: The result would be: Not only would this help with internationalisation, but it would mean that the quotation marks fit the style used elsewhere on a given website (assuming that |
|
Thanks, @pandammonium, for the feedback! I've looked for If only I had to add a caveat, the string We can create a PR to address it if we want to get some more feedback, as we will change the markup, and maybe it will affect some themes. But it is a really quick win, just changing the quotation marks
Do you want to address it? I will be glad to review it and help in case you need it 😄 |
|
Thanks for responding, @c4rl0sbr4v0! I agree with your caveat. A PR might be a good idea to see if I'd like to address it, but I don't know where to start 🤪 Please do help! |
By consequence, isn't this particular issue something that can be addressed by "translating" the quotation marks in the British English language pack (assuming that it's widely agreed upon that single quotes should be used here)? |
New Hart's Rules, a reputable style guide in the UK that deals with British, American and other varieties of English, states (§9.2.3):
It's true in practice: compare a book published in the UK to one published in the USA.
That might be a better idea, yes, although it looks to me like all the double quotation marks have been 'translated' to double ones already. Furthermore, I don't see this particular instance in the list of translations, and I don't know how to add it. (I'm not really familiar with translations, tbh.) |
What?
Make the Comments Title text non-editable by the user, i.e. hard-wire it to read "... responses to ...", while only retaining the option to display or hide the comments count and post title, respectively.
Why?
See this Slack discussion. Basically, using a
<PlainText />component with a placeholder to allow editing a string between a comment count and a post title breaks i18n.How?
In order to be able to use a properly i18n'd string (via
sprintf()with placeholders, _()and_n()), we have to say good-bye to allowing the user to edit that string, and instead add a bunch of conditionals to determine which i18n'd string to render, depending on whether or not we're showing the comments count and/or post title 😅As a consequence, we need to remove the
singleCommentLabelandmultipleCommentsLabelattributes. A deprecation and corresponding test fixture is included in the PR.Testing Instructions
trunk, open the Site Editor. Navigate to the templates list, and select the Single Post template for editing.replies to).One reply to.One response to(rather thanreply).singleCommentLabelandmultipleCommentsLabel) are gone.Screenshots or screencast