Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove colons from control labels #65205

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Remove colons from control labels #65205

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce86a8b
Remove colons from control labels in docs
mirka Sep 10, 2024
1ec54f8
Remove label colon from Archives block
mirka Sep 10, 2024
09db223
Remove label colon in Latest Posts block
mirka Sep 10, 2024
b57dcdc
Fix label in Legacy Widget block
mirka Sep 10, 2024
f267be0
Shorten to "Legacy widget" and update test
mirka Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference-guides/block-api/block-context.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ export default function Edit( props ) {
return (
<div>
<TextControl
label={ __( 'Record ID:' ) }
label={ __( 'Record ID' ) }
value={ recordId }
onChange={ ( val ) =>
setAttributes( { recordId: Number( val ) } )
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/archives/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export default function ArchivesEdit( { attributes, setAttributes } ) {
<SelectControl
__next40pxDefaultSize
__nextHasNoMarginBottom
label={ __( 'Group by:' ) }
label={ __( 'Group by' ) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before After
Archives block inspector, before Archives block inspector, after

options={ [
{ label: __( 'Year' ), value: 'yearly' },
{ label: __( 'Month' ), value: 'monthly' },
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/latest-posts/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ export default function LatestPostsEdit( { attributes, setAttributes } ) {
{ displayPostContent && (
<RadioControl
className="wp-block-latest-posts__post-content-radio"
label={ __( 'Show:' ) }
label={ __( 'Show' ) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before After
Latest Posts block inspector, before Latest Posts block inspector, after

selected={ displayPostContentRadio }
options={ [
{ label: __( 'Excerpt' ), value: 'excerpt' },
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/select-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ Render a user interface to select multiple users from a list.
```jsx
<SelectControl
multiple
label={ __( 'Select some users:' ) }
label={ __( 'User' ) }
value={ this.state.users } // e.g: value = [ 'a', 'c' ]
onChange={ ( users ) => {
this.setState( { users } );
Expand All @@ -126,7 +126,7 @@ const [ item, setItem ] = useState( '' );
// ...

<SelectControl
label={ __( 'Select an item:' ) }
label={ __( 'My dinosaur' ) }
value={ item } // e.g: value = 'a'
onChange={ ( selection ) => { setItem( selection ) } }
__nextHasNoMarginBottom
Expand Down
2 changes: 1 addition & 1 deletion packages/widgets/src/blocks/legacy-widget/edit/widget-type-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test, install and enable a legacy theme like Twenty Twelve. Go to wp-admin/widgets.php, and insert a Legacy Widget block into one of the sections.

Before After
Legacy Widget placeholder, before Legacy Widget placeholder, after

No strong opinion about the label text — I think it could be as short as "Legacy widget".

Copy link
Member

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:

https://github.com/GlotPress/GlotPress/blob/7c700cc9e76324460387259b01e38969ce1f074c/gp-includes/things/original.php#L450

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.

Copy link
Contributor

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?

Copy link
Member

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).

Copy link
Member Author

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?

Copy link
Member

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 👍

Copy link
Member Author

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.

value={ selectedId ?? '' }
options={ [
{ value: '', label: __( 'Select widget' ) },
Expand Down
Loading