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.

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

Don't programmatically lowercase post type label #49591

Merged
merged 1 commit into from
Apr 21, 2023
Merged

Don't programmatically lowercase post type label #49591

merged 1 commit into from
Apr 21, 2023

Conversation

tyrann0us
Copy link
Contributor

What?

Remove the programmatically lowercasing of the post type label in the "Link to %s" setting of the "Post Featured Image" block.

Fixes #49587

Why?

Programmatically lowercasing the label causes wrong grammar in languages that capitalize nouns like German.

How?

The .toLowerCase() call is removed. While this leads to capitalized post type labels also in English, IMO this acceptable. On multiple occassions throughout WordPress, English nouns are already capitalized, e.g., the "Update Profile" button on the user profile page:
Bildschirmfoto 2023-04-04 um 21 43 40

Also, the translators comment is changed to use "Page" because a) "post" is already used in the fallback string and b) the label is now capitalized.

Testing Instructions

  1. Insert the "Post Featured Image" to a page (not post)
  2. Open the block settings
  3. Verify that the setting reads "Link to Page" instead of "Link to page"
  4. Switch the locale to German
  5. Verify that the setting reads "Link zu Seite" instead of "Link zu seite"

Testing Instructions for Keyboard

Does not apply.

Screenshots or screencast

Before:
Bildschirmfoto 2023-04-04 um 21 08 32

After:
Bildschirmfoto 2023-04-04 um 21 45 39

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Apr 4, 2023
@github-actions
Copy link

github-actions bot commented Apr 4, 2023

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @tyrann0us! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@ramonjd ramonjd added the Internationalization (i18n) Issues or PRs related to internationalization efforts label Apr 12, 2023
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for putting together the PR to address this @tyrann0us 👍

I understand certain languages have strict rules regarding capitalization.

I'm not 100% sure simply stripping the toLowerCase() call here is the best option. There was a deliberate decision around control text being sentence cased (for English). @WordPress/gutenberg-design might be able to provide clarity here both in terms of direction and scope for whether removing the lowercase transform is fine.

The lowercase post type name is being passed through for translation which would allow translators to adjust the resulting text's capitalization e.g. from "page" to "Seite".

My only reservation on relying solely on translations would be that it could be possible for a custom post type to be registered with a non-English name and may not then have a translation to reinstate any capitalization.

My apologies for holding up this PR but it would be good to see if we can satisfy the needs of multiple languages rather than compromise one for the other.

@jasmussen
Copy link
Contributor

I'm not 100% sure simply stripping the toLowerCase() call here is the best option. There was a deliberate decision around control text being sentence cased (for English).

Thanks for the ping. Yes as a rule of thumb when authoring new contents in the default English language, we don't use titlecase for form controls. That's mainly title case, though, and as shown in the example, proper nouns in German are capitalized, so it seems incorrect to programmatically lowercase it. So I wouldn't personally mind removing that function for this particular use case.

The lowercase-only, in my best understanding, is a guideline for full phrases (such as __( 'Link to homepage' )), and in cases where those are pieced together we shouldn't transform them (i.e. __( 'Link to %s' )).

That being said, concatenating labels like these in general seems to be something to avoid. In the case of the link toggle, it is understandably necessary since featured images can sit on posts or pages etc. But whenever it's possible, it seems best for translation purposes to avoid concatenation, so the full context can be available to the translator.

@tyrann0us
Copy link
Contributor Author

Thank you for the feedback!

@aaronrobertshaw

The lowercase post type name is being passed through for translation which would allow translators to adjust the resulting text's capitalization e.g. from "page" to "Seite".

I might be overlooking something, but since the post type label is not passed to the translation function, but only referenced as a placeholder, how would a translator be able to change its capitalization? 🤔

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

I might be overlooking something, but since the post type label is not passed to the translation function, but only referenced as a placeholder, how would a translator be able to change its capitalization? 🤔

I misread that code sorry, you are correct 🤦

So I wouldn't personally mind removing that function for this particular use case.

It looks like we have a green light to remove the lowercase call.

I've taken this for a spin and it tests as advertised.

LGTM 🚢

@aaronrobertshaw
Copy link
Contributor

The failing e2e appears unrelated, so I've restarted that job.

Assuming it passes, I'll merge this before finishing up for the day.

@aristath aristath merged commit 03108c0 into WordPress:trunk Apr 21, 2023
@github-actions github-actions bot added this to the Gutenberg 15.7 milestone Apr 21, 2023
@bph bph added the [Type] Bug An existing feature does not function as intended label Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Internationalization (i18n) Issues or PRs related to internationalization efforts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Link to […]" should not lowercase post type label
6 participants