SkeletonView respecting Font's height, rather than the `UIView` actual height

RuiAAPeres commented 3 years ago

Description

I noticed this bug when I updated to 1.11.0. Let's assume the following setup for a simple UILabel:

isSkeletonable = true
font set to something with 20 points
height set to 5 points

Before 1.11.0, what would happen is that SkeletonView would respect the View's height and keep it at that 5 points height. With 1.11.0, the SkeletonView's height, will follow the font's height, instead of the explicitly set View's 5 points. A simple trick to mitigate this situation is to use clipsToBounds = true.

Am I missing something?

What type of issue is this? (place an `x` in one of the `[ ]`)

[X] bug
[ ] enhancement (feature request)
[ ] question
[ ] documentation related
[ ] discussion

Requirements (place an `x` in each of the `[ ]`)

[X] I've read and understood the Contributing guidelines and have done my best effort to follow them.
[X] I've read and agree to the Code of Conduct.
[X] I've searched for any related issues and avoided creating a duplicate issue.

Juanpe commented 3 years ago

Hi @RuiAAPeres, thanks for sharing this bug. I going to check it asap :)

RuiAAPeres commented 3 years ago

@Juanpe you are the best. I would just like to make a small amend to my original post. I might have implied that the fault is with 1.11.0, but it might not be the case.

Juanpe commented 3 years ago

Hi @RuiAAPeres, version 1.12.0 was released yesterday, and this issue must be resolved.

Don't hesitate to reopen or create a new one if you find some issue ;)

lordzsolt commented 3 years ago

Not sure if this is related, but after updating from 1.10.0 to 1.12.0, something got changed in how UILabel's skeletons are rendered, and adding clipsToBounds is needed. It would be great if the SkeletonViews clipped themselves.

Scenario: UILabel with font size 15. Min height constraint set to 20.

Before:

After:

Juanpe commented 3 years ago

Hi @lordzsolt,

The font size is ignored while calculating the skeleton height since version 1.12.0. But I think it's not correct at all, I mean the final height should consider both the font size and the height constraint. If the font size is greater than the height, then height will be used, otherwise, the font size will be used.

I will try to fix it as soon as possible to include it in the next version :)

RuiAAPeres commented 3 years ago

We have two scenarios here:

Label's height smaller than Font's height	Label's height bigger than Font's height

Independently of clipping, or not, a font that doesn't fit in its parent's height, is likely a developer mistake and not something SkeletonView should accommodate*. This is the reason why I think SkeletonView should approach this problem from a correctness point of view: honour the component's height.

*Can we agree that the first screenshot is a developer mistake?

lordzsolt commented 3 years ago

I completely agree with @RuiAAPeres. Just require people to put a min-height constraint (or non-empty contentSize by setting " ") in their labels, so SkeletonView can just take the height of the component.

The less "smart" SkeletonView tries to be, the more predictable the outcome.

Juanpe commented 3 years ago

mm, using the current version these examples, the skeleton layer looks like this:	Label's height smaller than Font's height	Label's height bigger than Font's height

And taking into account the solution that I provided previously, the skeleton would look like:	Label's height smaller than Font's height	Label's height bigger than Font's height

I agree with you that both scenarios are wrong, I mean, the developer should fit the label height to the font size. If we don't do any fixes, @lordzsolt must reduce the height of your labels in order to show the skeleton as it looked in the version 1.10.0.

WDYT?

lordzsolt commented 3 years ago

But my problem is, with the latest version, if the labels height is bigger than the font height, it doesn't look like the top right one.

It will be two lines, and the second line goes outside the bounds of the UILabel.

I'm actually not sure if "label height is bigger than font height" is a problem case. You will have this problem with multiline labels.

Juanpe commented 3 years ago

Ahh ok, I misunderstood the issue. So, your label has more than one line, right? In this case, the issue is not related to this one, the change was included in the version 1.11.0. Let me check then

Juanpe / SkeletonView