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

Fix min max ratio inconsistency in getMeasuredTextWidth #226

Merged
merged 3 commits into from
Aug 11, 2021

Conversation

JoePaul
Copy link
Contributor

@JoePaul JoePaul commented Aug 4, 2021

Hi again, I found an inconsistency in the code that I wrote in #214. In getMeasuredTextWidth I used coerceIn instead of coerceAtMost which is more consistent with how the other cases is written. The subtraction of spaces outside of the coerceIn-function gave some unwanted behavior where the dialog would be smaller than intended

@JoePaul JoePaul requested a review from skydoves as a code owner August 4, 2021 13:40
Copy link
Owner

@skydoves skydoves left a comment

Choose a reason for hiding this comment

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

Hi @JoePaul,
I left little comments for the PR.
Thanks for your contribution again! 😄

@@ -1226,7 +1226,7 @@ class Balloon(
val spaces = rootView.paddingLeft + rootView.paddingRight + if (builder.iconDrawable != null) {
builder.iconWidth + builder.iconSpace
} else 0 + builder.marginRight + builder.marginLeft + (builder.arrowSize * 2)
val maxTextWidth = displayWidth - spaces
val maxTextWidth = builder.maxWidth - spaces
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a bug with maxTextWidth in getMeasuredTextWidth that when we had longer texts we would get this behavior
image
when the expected behavior should be like the following
image

@skydoves skydoves self-requested a review August 11, 2021 08:06
Copy link
Owner

@skydoves skydoves left a comment

Choose a reason for hiding this comment

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

LGTM :)
Thanks, @JoePaul !

@skydoves skydoves merged commit 5935c30 into skydoves:main Aug 11, 2021
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