-
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
Improve strings quality and consistency #20187
Conversation
- Improve strings consistency for action labels with WP core pattern (Capitalize) - Merge with similar existing strings.
I believe we explicitly switched recently to sentence case everywhere. Can you confirm @karmatosed? |
In titles or button labels? |
@aduth I have not found anything suitable in the Core manual. Only this section. At the moment it is very inconsistent. In Gutenberg there are now some strings in different upper and lower case notations. The changes of the last weeks differ from the rest of the core project. Should we add this topic to a meeting? Or is there a documentation page I haven't found yet? |
That is why I purpose this in the first place, because WP core is a bit more consistent at the moment. |
Seems like it would be good to reconcile. The guidelines for core are a bit sparse and don't really make clear (at least in my reading) what "Capitalization" entails when there are multiple words. Maybe we can move or adapt the Gutenberg guidelines? To be clear, I don't have a preference one way or the other, just that there's consistency. Right now, it seems like there's conflicting consistencies between Gutenberg and the rest of core. A meeting might be the best place to resolve this. In the meantime, I would expect we probably either want to hold off on these specific changes, or conform to consistency with the Gutenberg guidelines and the rest of the codebase. |
native: __( 'ADD MEDIA' ), | ||
native: __( 'Add Media' ), |
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.
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
.
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.
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).
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'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.
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.
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!
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.
@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).
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.
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"?
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.
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.
Based on the linked precedent, it's my understanding that these changes would not be desirable, currently favoring sentence case per the following guidelines: See also:
Separately, we should discuss the inconsistencies between Gutenberg and the rest of wp-admin. I requested that this be discussed in tomorrow's (2020-02-26) editor meeting: https://make.wordpress.org/core/2020/02/25/editor-chat-agenda-26th-february-2020/#comment-37980 Based on the outcome of that discussion, it may or may not be warranted to revisit this, but as part of a larger effort of strings consistency. |
Fixes #20186
Description