-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove colons from control labels #65205
Conversation
@@ -53,7 +53,7 @@ export default function ArchivesEdit( { attributes, setAttributes } ) { | |||
<SelectControl | |||
__next40pxDefaultSize | |||
__nextHasNoMarginBottom | |||
label={ __( 'Group by:' ) } | |||
label={ __( 'Group by' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -243,7 +243,7 @@ export default function LatestPostsEdit( { attributes, setAttributes } ) { | |||
{ displayPostContent && ( | |||
<RadioControl | |||
className="wp-block-latest-posts__post-content-radio" | |||
label={ __( 'Show:' ) } | |||
label={ __( 'Show' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -29,7 +29,7 @@ export default function WidgetTypeSelector( { selectedId, onSelect } ) { | |||
<SelectControl | |||
__next40pxDefaultSize | |||
__nextHasNoMarginBottom | |||
label={ __( 'Select a legacy widget to display:' ) } | |||
label={ __( 'Legacy widget to display' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shorter one makes sense to me; however, note that if we change the string too much, GlotPress will drop existing translations and will no longer consider them "fuzzy." By default, if half the string is the same, its translations will be kept and marked as fuzzy:
With that in mind, what you changed should help keep the string. If you change it to "Legacy widget", the translations will be dropped and the string will have to be re-translated. This might be what you essentially expect, but I just wanted to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Legacy widget" is much better. Could we maybe ping someone in the translation community to take care of it after merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translations are a shared responsibility, thousands of folks translate into all languages, and the string will need to just be re-translated in all languages. Not a huge deal, it's happened before. Just feels like something that's not part of the PR's goal and will cause an unintended side effect (losing the existing translations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I wasn't thinking about translations! I'm not super familiar with the i18n system, but since there already is a "Legacy widget" string within the same project, I would assume that it would just reuse that? @tyxla Do you happen to know if there are any quirks here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, didn't realize we're using that string already. It appears like it should work! It would only be an issue if the meaning of the string in the two separate contexts is not the same. Looks like it should be OK though 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, let's go with the shorter one then.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -11.7 kB (-0.66%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Confirmed there are no remaining occurrences of labels with trailing colons.
👌 |
What?
Removes any trailing colons in the labels of control components.
Why?
When doing consistency work across the core blocks, I noticed that there are some blocks that have a trailing colon in the label of some of their controls. These are unnecessary and goes against the labeling style of everything else. (This should probably be part of a style guide — cc @mattrwalker @WordPress/gutenberg-design)
Testing Instructions
See inline comments.