search

grantland / android-autofittextview

A TextView that automatically resizes text to fit perfectly within its bounds.
Apache License 2.0
4.27k stars 689 forks source link

Added height fitting option (by KonradJanica) #69

Open Will5 opened 8 years ago

Will5 commented 8 years ago

Took KonradJanica's code (#43) and reformatted for easier review by grantland.

grantland commented 8 years ago

This is much cleaner!

The new API method names seem a bit odd to me. Instead of #isHeightFitting(), #setHeightFitting(boolean), etc. do you think #isAutofitWidthEnabled(), setAutofitHeightEnabled(boolean) sounds more natural? Additionally, this would allow us to eventually deprecate #isEnabled(), #getEnabled(boolean) and add #isAutofitWidthEnabled() and #setAutofitWidthEnabled(boolean).

Will5 commented 8 years ago

The proposed naming convention of #isAutofitHeightEnabled() and #setAutofitHeightEnabled(boolean), along with #isAutofitWidthEnabled() and #setAutofitWidthEnabled(boolean) sound much cleaner to me!

Will5 commented 8 years ago

I pushed a commit with the API refactoring. Let me know if it sounds clearer.

AllanHasegawa commented 8 years ago

Awesome, looks good ^^

Thank you for your commits.

Just a small nitpick. The variable named mIsAutofitHeightEnabled is using "Hungarian Notation" to denote it is a "member variable". See Google's code style: link.

When you use it inside a static function (like the autofit(...)), I think the proper naming convention would be to remove the "m" before the variable name, as it is no longer a "member variable".

Will5 commented 8 years ago

Thank you @AranHase!

ejohansson commented 7 years ago

Any updates on this. Have been checking in on this pull request for a while now?

johnnylambada commented 7 years ago

I made a quick fork of this with @Will5 's changes because I need to use it in my project. It's available from jitpack.io here: https://github.com/flipagram/android-autofittextview -- When this is all merged in I'll remove my fork.

slvrfn commented 7 years ago

why has this still not been merged?

AndreasBoehm commented 7 years ago

Please merge this PR, we do need this fix for our project as well. Thanks!