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

[ASTextNode2] Excluding origin from calculating text node size #1149

Closed
wants to merge 2 commits into from
Closed

[ASTextNode2] Excluding origin from calculating text node size #1149

wants to merge 2 commits into from

Conversation

ernestmama
Copy link
Contributor

@ernestmama ernestmama commented Sep 28, 2018

ASTextLayout would include the origin to the width, it does not work when the text node is layout with other layoutspec like StackLayout and when the container is provided with an ambiguous size.

@ghost
Copy link

ghost commented Sep 28, 2018

🚫 CI failed with log

@ghost
Copy link

ghost commented Sep 29, 2018

1 Warning
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@ernestmama ernestmama changed the title Excluding origin from calculating text node size [ASTextNode2] Excluding origin from calculating text node size Oct 1, 2018
@nguyenhuy
Copy link
Member

@Adlai-Holler Can you take a look at this PR when you have time?

@nguyenhuy
Copy link
Member

@ernestmama You're right that the problem is caused when ASTextLayout takes right/center/natural text alignment into account even when the max width is artifically large/inf. ASTextNode handles this correctly, but ASTextNode2 doesn't.

However, I believe your fix can cause other problems, such as making ASTextNode2 to no longer correctly support these alignments. I proposed another fix (#1166) that is a bit more surgical and should work nicely with existing code, including ASTextLayout's cache.

@ernestmama ernestmama closed this Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants