Update npm packages to latest versions for 6.4 Beta 3 #5439
Conversation
There was a problem hiding this comment.
@mikachan Mostly LGTM, one concern about i18n here (@swissspidy can you advise?), and 2 non-blocking documentation nit-picks.
| * | ||
| * @return void |
There was a problem hiding this comment.
@return void shouldn't be used in core.
| * | |
| * @return void |
There was a problem hiding this comment.
@felixarntz could you elaborate on this? Searching the codebase turns up quite a few uses of @return void. In what circumstances should it not be used?
There are a few instances across core block files, so if we do remove them we should probably do them all in one go.
There was a problem hiding this comment.
@tellthemachines @mikachan Any @return void found in core were probably missed by reviewers, or reviewers weren't aware of the requirement. See https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/#:~:text=Note%3A%20%40return%20void%20should%20not%20be%20used%20outside%20of%20the%20default%20bundled%20themes.
If we can remove those consistently, that would be great.
There was a problem hiding this comment.
Here's a PR: WordPress/gutenberg#55237
| * | ||
| * @return void |
There was a problem hiding this comment.
See above.
| * | |
| * @return void |
| $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' ), | ||
| esc_html( $title ) | ||
| ); |
There was a problem hiding this comment.
Not sure if this is good from a i18n perspective. Specifically:
- Does it make sense for the
…to be part of the (same) translation string? - I think the "Read more" must not be outside of this main string, otherwise context is missing.
- Should we move the
atag outside the translation string and wrap it instead? - The "Read more: {post title}" is a bit of a weird format text. Most core themes have solved this via something like
sprintf( __( 'Continue reading %s' ), $title ).
Maybe something like this would be better:
| $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' ), | |
| esc_html( $title ) | |
| ); | |
| $trimmed_excerpt .= '… <a href="' . esc_url( $post_link ) . '" rel="noopener noreferrer">'; | |
| $trimmed_excerpt .= sprintf( | |
| /* 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>'; |
There was a problem hiding this comment.
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.
Currently AFK, so I definitely do recommend checking similar instances in core themes and elsewhere, and asking @SergeyBiryukov or #polyglots if in doubt.
There was a problem hiding this comment.
These suggestions make sense to me, thank you! I've addressed them here, but it would be great to get some feedback from more experienced i18n folks too.
There was a problem hiding this comment.
Discussion probably could continue on GB55026.
This is a pair of similar strings. "Read more" already has been visible on the front end since 6.3, so we may need to keep that exact text in the string.
|
Committed in https://core.trac.wordpress.org/changeset/56808. |
Trac ticket: https://core.trac.wordpress.org/ticket/59411
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.