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

Rename TextBlock to TextLayout #15797

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Rename TextBlock to TextLayout #15797

merged 2 commits into from
Oct 9, 2024

Conversation

UkoeHB
Copy link
Contributor

@UkoeHB UkoeHB commented Oct 9, 2024

Objective

Solution

  • Rename TextBlock to TextLayout.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 9, 2024
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Text Rendering and layout for characters X-Contentious There are nontrivial implications that should be thought through S-Needs-SME Decision or review from an SME is required labels Oct 9, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I prefer this naming. I think this is much clearer: it's more adjective-y, rather than being a distinct noun.

@cart
Copy link
Member

cart commented Oct 9, 2024

From my perspective this should still be a "driving concept" with a noun. TextLayout feels like it is "just" describing the text layout configuration. But imo we can (and should) make this the driving concept for "thing that receives text spans and renders them". TextBlock feels like the better name in that context.

@cart
Copy link
Member

cart commented Oct 9, 2024

But if we commit to this just being "text layout configuration" from a user perspective, this rename is fine

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed X-Contentious There are nontrivial implications that should be thought through labels Oct 9, 2024
@UkoeHB
Copy link
Contributor Author

UkoeHB commented Oct 9, 2024

Right now the requires are

  • Text -> {TextStyle, TextBlock -> {ComputedTextBlock, TextLayoutInfo}}.

What if we rearrange like this

  • Text -> {TextStyle, ComputedTextBlock -> {TextLayout, TextLayoutInfo}} or
  • Text -> {TextStyle, TextLayout, ComputedTextBlock -> {TextLayout, TextLayoutInfo}}?

It feels quite wrong to me to make a TextBlock component to set text layout configs, just because the name TextBlock fits into some unrelated semantic structure.

@cart
Copy link
Member

cart commented Oct 9, 2024

Hmm that would have conceptual / ergonomic implications when trying to use the "TextBlock" concept (in your proposal ComputedTextBlock) directly:

commands.spawn((Node, ComputedTextBlock, TextLayout::new_with_justify(JustifyText::Center))

Vs

commands.spawn((Node, TextBlock::new_with_justify(JustifyText::Center))

That being said, with the current impl this standalone usage is not currently possible, and I'm not really sure we even need it.

In the short term I'm cool with TextLayout, provided it is used "just" for config. I agree that the introduction of a "non layout" concept does muddle things when that new concept isn't currently actually used anywhere outside of layout. If we later decide that TextBlock is a useful user facing concept, we can re-open the discussion.

@cart cart added this pull request to the merge queue Oct 9, 2024
@cart cart removed the X-Controversial There is active debate or serious implications around merging this PR label Oct 9, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers and removed S-Needs-SME Decision or review from an SME is required labels Oct 9, 2024
Merged via the queue into bevyengine:main with commit a6be9b4 Oct 9, 2024
29 checks passed
@UkoeHB UkoeHB deleted the text_layout branch October 9, 2024 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants