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

Text: Add text-wrap: pretty #60164

Merged
merged 4 commits into from
Apr 4, 2024
Merged

Text: Add text-wrap: pretty #60164

merged 4 commits into from
Apr 4, 2024

Conversation

 jasmussen
Copy link
Contributor

What?

Alternative to, and followup, to #58728 .

Adds text-wrap: pretty; to the <Text> component, to avoid typographic widows.

 code sample showing the value

Testing Instructions

Test using the inspector to edit the block card and try to add words to make a typographic widow happen. It shouldn't be possible:

 block card

 @jasmussen jasmussen added the [Type] Enhancement A suggestion for improvement. label Mar 25, 2024
 @jasmussen jasmussen self-assigned this Mar 25, 2024
Copy link

github-actions bot commented Mar 25, 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: jasmussen < joen@git.wordpress.org > Co-authored-by: mirka < 0mirka00@git.wordpress.org > Co-authored-by: t-hamano < wildworks@git.wordpress.org >

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

Copy link
Contributor

 @t-hamano t-hamano 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 the PR.

This change makes sense to me, but it will affect all developers using this component, so I'd like to hear what the component team has to say.

 @t-hamano t-hamano requested a review from a team March 26, 2024 13:29
 @t-hamano t-hamano added the [Package] Components /packages/components label Mar 26, 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 .

I'm still concerned about the overall cost in terms of performance. I've looked into it a bit ( MDN , Chrome Blog ), and they all warn that this is something you should use when you specifically want good typography over speed.

The Text component is very generic, and I'm not sure we should make that judgement call for everybody. Of course we can try out this change and monitor Code Vitals, but it's harder to control how much more Text will be widely used in the future, or in third-party apps.

Are there any specific parts of the UI where we want to see pretty text-wrap (i.e. "choose beauty over speed")? Or do we really want to choose beauty over speed everywhere for everybody? It may be better to start with more specific parts of the UI.

 @jasmussen
Copy link
Contributor Author

jasmussen commented Apr 1, 2024

this is something you should use when you specifically want good typography over speed.

For me that choice is trivially easy : typography :)

But that assumes that the speed choice is as small in impact as I have perceived it to be on all the websites that already apply this. That is, negligible. But admittedly, that's a data point of 1 person not looking too deeply, and running a fast laptop. I wonder is there any way we can find out more about this performance hit? I added it to this component based on a specific suggestion .

Are there any specific parts of the UI where we want to see pretty text-wrap (i.e. "choose beauty over speed")? Or do we really want to choose beauty over speed everywhere for everybody? It may be better to start with more specific parts of the UI.

Personally I'd like to see this applied to any place where we a paragraph longer than a single line is applied. I recognize that's pretty broad, but intentionally so. But that would include:

  • Descriptions in the inspector, or the dark material sidebar on the left.
  • Text pages in WordPress, such as the About page, privacy policy, all the other major sections.
  • Settings pages.
  • Content inside modals.

 @mirka
Copy link
Member

I could find and confirm these tests results , where the render time difference is negligible even with a simulated 6x CPU slowdown. Also, the description of the algorithm seems to say that we won't see much of a performance hit on instances that wouldn't benefit from the better breaking algorithm.

Another good thing is that if we ever have to revert, I think we could get away with it without calling it a breaking change.

So I think I'm ok with trying it out as a blanket style on Text , once we update the snapshots and add a changelog 👍

 @jasmussen jasmussen force-pushed the try/text-wrap-pretty branch 2 times, most recently from 3e173d3 to 52b135b Compare April 3, 2024 07:56
 @jasmussen
Copy link
Contributor Author

If you're okay with this one, I could use a ✅

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 .

Good to go 👍

 @mirka mirka merged commit be4e95a into trunk Apr 4, 2024
66 checks passed
 @mirka mirka deleted the try/text-wrap-pretty branch April 4, 2024 22:20
 @github-actions github-actions bot added this to the Gutenberg 18.2 milestone Apr 4, 2024
 @jasmussen
Copy link
Contributor Author

Thanks so much for the help 🙏

cbravobernal pushed a commit to garridinsi/gutenberg that referenced this pull request Apr 9, 2024
 * Text: Add text-wrap: pretty * Update changelog. * Move changelog item to enhancements. * Update snapshots --------- Co-authored-by: jasmussen < joen@git.wordpress.org > Co-authored-by: mirka < 0mirka00@git.wordpress.org > Co-authored-by: t-hamano < wildworks@git.wordpress.org >
Sign up for free to join this conversation on GitHub . Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants