Skip to content

Improve strings quality and consistency #20187

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

Closed
pedro-mendonca wants to merge 1 commit into from
Closed

Improve strings quality and consistency #20187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 packages/block-library/src/button/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ function ButtonEdit( {
checked={ linkTarget === '_blank' }
/>
<TextControl
label={ __( 'Link rel' ) }
label={ __( 'Link Rel' ) }
value={ rel || '' }
onChange={ onSetLinkRel }
/>
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/gallery/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const PLACEHOLDER_TEXT = Platform.select( {
web: __(
'Drag images, upload new ones or select files from your library.'
),
native: __( 'ADD MEDIA' ),
native: __( 'Add Media' ),
Copy link
Member

@aduth aduth Feb 14, 2020

Choose a reason for hiding this comment

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

This one would probably still be good to fix regardless. I'm not really sure why this label is fully-capitalized?

cc @mkevins @pinarol @jorgefilipecosta re: #18265

If it's a UI thing, I'd wonder if there's some native equivalent to the web's text-transform: uppercase.

Copy link
Contributor

@mkevins mkevins Feb 17, 2020

Choose a reason for hiding this comment

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

This all-caps string was brought in by this commit: abd295d to be consistent with the other native media blocks:

gutenberg/packages/block-editor/src/components/media-placeholder/index.native.js

Lines 75 to 83 in 78de49e

if ( instructions === undefined ) {
if ( isImage ) {
instructions = __( 'ADD IMAGE' );
} else if ( isVideo ) {
instructions = __( 'ADD VIDEO' );
} else {
instructions = __( 'ADD IMAGE OR VIDEO' );
}
}

I believe the text-transform property (textTransform on React Native) is now supported on both iOS and Android, so the native MediaPlaceholder could be refactored to use that. I'm not 💯 sure, but it may be that this was previously not possible (at the time these strings were introduced).

Copy link
Contributor

@pinarol pinarol Feb 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not really sure why this label is fully-capitalized?

It was a design decision. (@iamthomasbishop correct me if I am wrong.)

I believe the text-transform property (textTransform on React Native) is now supported on both iOS and Android, so the native MediaPlaceholder could be refactored to use that. I'm not 💯 sure, but it may be that this was previously not possible (at the time these strings were introduced).

This is tricky mostly because of non technical reasons. Our translation rates aren't perfect in some languages. Thus, if we use device locale to do the uppercasing, it might generate some unexpected results on strings that are not localized yet. In general, we don't usually do uppercasing/lowercasing for user facing Strings on mobile. Also these kind of uppercase strings are very rarely used.

portfolio_view

Copy link
Contributor

@mkevins mkevins Feb 17, 2020

Choose a reason for hiding this comment

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

This is tricky mostly because of non technical reasons. Our translation rates aren't perfect in some languages. Thus, if we use device locale to do the uppercasing, it might generate some unexpected results on strings that are not localized yet.

Thanks for that extra context!

Copy link
Member

@aduth aduth Feb 25, 2020

Choose a reason for hiding this comment

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

@pinarol I'm a bit unclear on whether you're suggesting we cannot or should not use textTransform? In my mind, the device transform would have the same result as the current implementation whether or not the string is translated, and I would think it would be preferable to err on the side of not ascribing that case in the string used in code (for consistency, and for improved reusability of that string).

Copy link
Contributor

@pinarol pinarol Feb 25, 2020

Choose a reason for hiding this comment

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

In some languages the uppercase of some letter is different than their uppercase in English. Like in Turkish, the uppercase of "i" is "İ", and the lowercase of "I" is "ı". So if we programmatically uppercase "Add Media" using TR locale it'll become "ADD MEDİA" which is not correct. This wouldn't be a problem if we had good translation support on every language but in some languages users see a mixture of English & X language words. So we'll be uppercasing English words using X locale, which is why I want to avoid using dynamic uppercasing/lowercasing.

But I agree that we should aim for improved reusability, so let me loop in @iamthomasbishop and see what he thinks about dropping the fully uppercase "ADD MEDIA" in favor of "Add Media"?

Copy link

@iamthomasbishop iamthomasbishop Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

From a native-mobile perspective, I would definitely prefer writing the strings in one way (sentence-case to align w/ Core) and programatically transform based on the target platform's component/label guidelines.

In other words, we'd always write the string as sentence-case (for example: Xxx xxxxx or Add media). From there, we would transform casing on RN based on the platform guidelines for the corresponding component (button == uppercase on Android and title-case on iOS, whereas a normal text-label title would be sentence-case on Android and title-case on iOS).

Does this make sense? I also remember someone mentioning that we aren't able to reliably use text-transform on RN, but not sure if that's resolved.

} );

// currently this is needed for consistent controls UI on mobile
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/navigation-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ function NavigationLinkEdit( {
<ToolbarButton
name="submenu"
icon={ <ToolbarSubmenuIcon /> }
title={ __( 'Add submenu' ) }
title={ __( 'Add Submenu' ) }
onClick={ insertLinkBlock }
/>
</ToolbarGroup>
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/post-taxonomies/flat-term-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ class FlatTermSelector extends Component {
const newTermLabel = get(
taxonomy,
[ 'labels', 'add_new_item' ],
slug === 'post_tag' ? __( 'Add new tag' ) : __( 'Add new Term' )
slug === 'post_tag' ? __( 'Add New Tag' ) : __( 'Add New Term' )
);
const singularName = get(
taxonomy,
Expand Down
8 changes: 4 additions & 4 deletions packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,13 +423,13 @@ class HierarchicalTermSelector extends Component {
);
const newTermButtonLabel = labelWithFallback(
'add_new_item',
__( 'Add new category' ),
__( 'Add new term' )
__( 'Add New Category' ),
__( 'Add New Term' )
);
const newTermLabel = labelWithFallback(
'new_item_name',
__( 'Add new category' ),
__( 'Add new term' )
__( 'Add New Category' ),
__( 'Add New Term' )
);
const parentSelectLabel = labelWithFallback(
'parent_item',
Expand Down