useFontLineHeight property not working as intended

ankitagarwal007 commented 3 years ago

Description

After the introduction of useFontLineHeight, and setting it true it is still not considering fontlineHeight.

var lineHeight: CGFloat { let constraintsLineHeight = backupHeightConstraints.first?.constant ?? SkeletonAppearance.default.multilineHeight

    if useFontLineHeight,
       let fontLineHeight = font?.lineHeight {
        return fontLineHeight > constraintsLineHeight ? constraintsLineHeight : fontLineHeight
    } else {
        return constraintsLineHeight
    }
}

in the above code lets say we don't have any height constraint so backupHeightConstraints will be nil, hence constraintsLineHeight will have value of SkeletonAppearance.default.multilineHeight(15) now if useFontLineHeight is true , lets say fontLineHeight value is 24 and (constraintsLineHeight is 15 due to SkeletonAppearance.default.multilineHeight) the condition says if fontLineHeight > constraintsLineHeight return constraintsLineHeight so in this it will return 15 always. This condition should be executed only if backupHeightConstraint has a value to give priority to constraint.

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.

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

SkeletonView Environment:

SkeletonView version: Xcode version: Swift version:

Steps to reproduce:

Please replace this with the steps to reproduce the behavior.

1. 2. 3.

Expected result:

Please replace this with what you expected to happen.

Actual result:

Please replace this with of what happened instead.

Attachments:

Logs, screenshots, sample project, funny gif, etc.

Juanpe commented 3 years ago

Hi @ankitagarwal007, you're right but I think it makes sense, I mean, if the font line height is bigger than the label's frame, we should use the label frame in order to avoid visual issues.

We already discussed about that here #353

WDYT?

stale[bot] commented 2 years ago

🤖 This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions 🙂

Juanpe / SkeletonView