Revise `useFontLineHeight` to also contemplate attributed text

[literallysofia]

Describe the feature or problem you’d like to solve

Currently, useFontLineHeight is used to determine how we want to define the height of a text's skeleton view (per line). The logic present at SkeletonTextNode:41 is a bit confusing and sensitive to edge cases. First, the variable constraintsLineHeight doesn't make sense to fallback to a constant in case there are no height constraints. The name is deceiving as it is and if we look at the following line:

fontLineHeight > constraintsLineHeight ? constraintsLineHeight : fontLineHeight

If there are no height constraints, then constraintsLineHeight will be 15, but the developer specified that he wants to use font's line height (useFontLineHeight = true, which it is by default too). fontLineHeight will only be used if it's less than 15, in this case. It doesn't make much sense to me... As discussed in #352, I agree that when we have constraints we should obey them regardless of the font. But what happens when they don't exist?

In addition to this issue, if we can use the font's line height to define the skeleton's height, then we should also be able to use the attributed text's line height. In the app that I'm working on we use mostly attributed text for labels, so it would be nice to have the possibility of using its line height.

Proposed solution

Improve the code in SkeletonTextNode:41 and make useFontLineHeight contemplate attributed text too (if so, the name should be something like useTextLineHeight).

multilineHeight should only be used if there are no height constraints, useFontLineHeight is false, and if by some reason the line height from font/attributed text doesn't exist. It is essentially the default if anything goes wrong/is not set, which I think it is how you intended it to be.

Let me know that you think! I'm hoping this wasn't too confusing 😄🤞

[literallysofia]

I investigated this a little further... Given a label with 1 line, if we use multilineHeight to define its height and if its value is somewhat greater than the label/layer's height, it will break. What I mean by breaking is: it wont have much styling, no border radius, correct width nor height (it downsizes to 12).

To replicate this issue, you can simply change the number of lines and make the label's skeleton view use multilineHeight.

3 lines; multilineHeight = 20	1 line; multilineHeight = 20	1 line; multilineHeight = 16

I would appreciate if you could give me a hand to fully understand the logic behind this. I guess this case, adding up to the others, confirms that this part is a bit shaky and has room for improvement! 😉

[Juanpe]

Hi @literallysofia 👋🏼

Thanks for this awesome investigation 👏🏼 I agree with you that the current code couldn't be working fine and the result couldn't be the desired 😕

The current code is there to prevent using a font height bigger than the frame. For instance, a label with a font size of 40pt and the height defined by the frame (constraints) is 30px. In this case, the library decides to use 30 instead of the font size, even though useFontLineHeight is true. But what happens when there are no defined constraints 🤔

So, as you said, this is a complex part of the library, and it could be improved 🤔 Let me think about that so as to try to find a better solution. If you have a definitive proposal, please, let me know :)

BTW, 100% with this:

Improve the code in SkeletonTextNode:41 and make useFontLineHeight contemplate attributed text too (if so, the name should be something like useTextLineHeight).

Thanks again :)

[Juanpe]

Hi @literallysofia 👋🏼

I've created this PR to improve this feature. https://github.com/Juanpe/SkeletonView/pull/466

[literallysofia]

Hi @Juanpe !!

Sorry for the late reply. Thanks for picking this up 👌 Definitely an improvement!

[Juanpe]

Great! I'm going to close this issue. Feel free to open a new one if you find something related :)