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

Menu.ItemHelpText: better line breaking #67011

Conversation

himanshupathak95
Copy link
Contributor

Fixes: #66998

What?

Changed CSS word-break property from break-all to break-word in meta fields for better text readability when content wraps.

Why?

Currently, text in meta fields breaks incorrectly within words (e.g., "white" breaks as "whit e") due to using break-all. This creates poor readability, especially for descriptions and long text content. Using break-word ensures text breaks at appropriate word boundaries.

Testing Instructions

  1. Add this test code to create a custom post type with meta fields in functions.php in your theme:
function init_test_meta_fields() {
   register_post_type(
       'landmark',
       array(
           'label'        => 'Landmark',
           'public'       => true,
           'supports'     => array( 'title', 'editor', 'comments', 'revisions', 'trackbacks', 'author', 'excerpt', 'page-attributes', 'thumbnail', 'custom-fields', 'post-formats' ),
           'has_archive'  => true,
           'show_in_rest' => true,
       ),
   );
   register_meta(
       'post',
       'landmark_description',
       array(
           'object_subtype' => 'landmark',
           'label'          => 'Description',
           'show_in_rest'   => true,
           'single'         => true,
           'type'           => 'string',
           'default'        => 'Inspired by the Eiffel Tower, this red and white tower is one of Tokyo\'s most recognizable landmarks.',
       )
   );
}

add_action( 'init', 'init_test_meta_fields' );
  1. Create a new Landmark post
  2. Check the attributes >> content desctiption field in meta fields
  3. Verify text now breaks correctly at word boundaries
  4. Test with various content lengths including URLs and long words

Your paragraph text

Screenshots

Before word-break: break-all; :

1

After word-break: break-word; :

2

Changed break-all to use word-break for better readability.
Copy link

github-actions bot commented Nov 14, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: himanshupathak95 <[email protected]>
Co-authored-by: juanfra <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: im3dabasia <[email protected]>
Co-authored-by: jasmussen <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

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

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @himanshupathak95! In case you missed it, we'd love to have you join us in our Slack community.

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

@mirka mirka requested a review from a team November 14, 2024 22:00
@mirka mirka added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Nov 14, 2024
Copy link
Member

@juanfra juanfra 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 your PR @himanshupathak95

It's testing nicely to me.

Screen.Recording.2024-11-19.at.18.12.50.mov

Can you please add an entry on the unreleased section of the changelog for components?

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

We can see that the original rule was put in place (#63916) with a pretty specific case in mind — a URL with a hyphen in the middle, like https://m.media-amazon.com/images/M/MV5BOTI5ZTNkYWQtNDg2Mi00MTBmLTliMGItNTI5YWI5OTZkM2Y2XkEyXkFqcGdeQXVyNzU1NzE3NTg@._V1_QL75_UX500_CR0,47,500,281_.jpg.

I do understand that break-all is not acceptable for wrapping (even if it's fine for truncation) in normal text, so I think reverting to word-break: normal is sensible.

Though we will see non-ideal wrapping for certain URLs, I think it's an acceptable compromise as long as they don't overflow and break the container, as was the original intent of the break-all:

A URL with a hyphen wraps in an non-ideal way

cc @ciampo for a gut check.

@@ -380,7 +380,7 @@ export const MenuItemHelpText = styled( Truncate )`
font-size: ${ font( 'helpText.fontSize' ) };
line-height: 16px;
color: ${ LIGHTER_TEXT_COLOR };
word-break: break-all;
word-break: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

word-break: break-word is deprecated, so we should use the non-deprecated equivalent:

Suggested change
word-break: break-word;
overflow-wrap: anywhere;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mirka, Thanks for the suggestion.

I have made the suggested changes. Please let me know if there is anything else that can be done.

Replace deprecated `word-break: break-word` with the modern equivalent
`overflow-wrap: anywhere` to handle text wrapping. This change maintains
the same text wrapping behavior while using the current standard CSS
property.
@himanshupathak95
Copy link
Contributor Author

Can you please add an entry on the unreleased section of the changelog for components?

Hey @juanfra, Thanks for replying.

I am not really sure what entry is to be made. I'd really appreciate it if you could steer me in the right direction.

@juanfra
Copy link
Member

juanfra commented Nov 20, 2024

I am not really sure what entry is to be made. I'd really appreciate it if you could steer me in the right direction.

@himanshupathak95 When making changes to WordPress components, it’s important to add a changelog entry in packages/components/CHANGELOG.md. At the top of this file, you’ll find an “Unreleased” section, which is where you should add your entry. This ensures the changes are included in the next release.

Within the “Unreleased” section, there are sub-sections like “Bug Fixes” and “Enhancements.” Add your entry under the most appropriate sub-section, following the standard format:

-   `ComponentName`: Description of the change ([#PR-ID](URL-OF-PR)).

@himanshupathak95
Copy link
Contributor Author

Thanks, @juanfra for the suggestion.

I have now added the appropriate entry in CHANGELOG.md.

Please let me know if there is anything else that can be done.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

The fix seems sensible to me, thank you for the ping @mirka !

Left a couple of minor comments, we can merge this PR once they are addressed

@@ -14,6 +14,7 @@
- `ToggleGroupControl`: Fix active background for `0` value ([#66855](https://github.com/WordPress/gutenberg/pull/66855)).
- `SlotFill`: Fix a bug where a stale value of `fillProps` could be used ([#67000](https://github.com/WordPress/gutenberg/pull/67000)).
- `ColorPicker`: Add accessible label for copy button ([#67094](https://github.com/WordPress/gutenberg/pull/67094)).
- `MenuItemHelpText`: Fix text wrapping to prevent unintended word breaks by using `overflow-wrap: anywhere` ([#67011](https://github.com/WordPress/gutenberg/pull/67011)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: adding a dot to represent the current component name

Suggested change
- `MenuItemHelpText`: Fix text wrapping to prevent unintended word breaks by using `overflow-wrap: anywhere` ([#67011](https://github.com/WordPress/gutenberg/pull/67011)).
- `Menu.ItemHelpText`: Fix text wrapping to prevent unintended word breaks by using `overflow-wrap: anywhere` ([#67011](https://github.com/WordPress/gutenberg/pull/67011)).

@@ -14,6 +14,7 @@
- `ToggleGroupControl`: Fix active background for `0` value ([#66855](https://github.com/WordPress/gutenberg/pull/66855)).
- `SlotFill`: Fix a bug where a stale value of `fillProps` could be used ([#67000](https://github.com/WordPress/gutenberg/pull/67000)).
- `ColorPicker`: Add accessible label for copy button ([#67094](https://github.com/WordPress/gutenberg/pull/67094)).
- `MenuItemHelpText`: Fix text wrapping to prevent unintended word breaks by using `overflow-wrap: anywhere` ([#67011](https://github.com/WordPress/gutenberg/pull/67011)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Another nit, but I think that the ### Internal section would be more fitting for this CHANGELOG entry, given that the component is still a private API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ciampo, Thanks for the suggestions.

I have addressed all the feedback. Please let me know if there is anything else that can be done.

@ciampo ciampo changed the title Fix: Use break-word instead of break-all in attribute description Menu.ItemHelpText: better line breaking Nov 20, 2024
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the follow-ups!

@mirka mirka merged commit 9a6f48b into WordPress:trunk Nov 21, 2024
64 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.8 milestone Nov 21, 2024
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 [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu: Truncation breaks with break-all rather than break-word
4 participants