Fix #322: Adjust height for popup when if it clips off screen

[danielbogomazov]

Fixes issue 322

Opening a popup in landscape mode would clip the popup off the bottom of the screen for smaller devices. This happened because the height of the popup is set based on the contents of the popup without adjusting for the screen's height.

Pull Request Checklist

• My patch has gone through review and I have addressed review comments

• My patch has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup!

• I have updated the Unit Tests to cover new or changed functionality

• I have updated the UI Tests to cover new or changed functionality

• I have marked the bug with [needsuplift]

• I have made sure that localizable strings use NSLocalizableString()

Screenshots

[kylehickinson]

Shouldn't call layoutSubviews directly. Instead make this call setNeedsLayout and additionally layoutIfNeeded if layout needs to happen instantly

[kylehickinson]

I know it's probably never going to be a problem, but is it possible to get a solution that doesn't involve potential recursion? :)

[danielbogomazov]

Thanks for the heads up. I've removed the recursion. Let me know what you think :)

[jhreis]

Thanks so much for submitting a PR! This will be a really nice thing to have, just a few random comments. Please let us know if you have any questions!

[jhreis]

This logic is actually functionally equivalent to the if conditional logic right above it. Probably cleaner to create this constant outside the if and just use it in the boolean logic:

let desiredHeight: CGFloat = UIScreen.main.bounds.height - externalContentHeight
if containerView.frame.height > desiredHeight {
    // use `desiredHeight`
}

[jhreis]

Remove all unneeded manual type setting (e.g. the 3 CGFloat instances)

[jhreis]

• Remove leading _
• Make resizePercentage a named parameter (no leading _ here either.

[jhreis]

Remove image forced casting (can unwrap inside the above if statement:

if let dialogImage = dialogImage, let dialogeImageSize = dialogImage.image?.size {

[jhreis]

Removed line break, think line is still decent length (< 120 at least), especially after removing the unneeded CGFloat.

[jhreis]

I've been thinking about this a bit more, and I think we can actually calculate all the necessary variables and setup what is needed before ever calling updateSubviews. Something like:

        let paddingHeight = padding * 3.0
        let externalContentHeight = dialogButtons.count == 0 ? paddingHeight : kPopupDialogButtonHeight + paddingHeight
        let desiredHeight = UIScreen.main.bounds.height - externalContentHeight

        let resizingRequired = containerView.frame.height > desiredHeight

and then use resizingRequired for the boolean related logic, thoughts @danielbogomazov?

[danielbogomazov]

Thanks for the review, @jhreis! I've fixed all of the commented blocks.

I'm not quite sure what you mean by

I think we can actually calculate all the necessary variables and setup what is needed before ever calling updateSubviews

The variables required are only relevant after the subviews are updated initially. They then check if the new height can fit on the screen - if not, it will resize them again, this time to fit. This means that desiredHeight can only be calculated after the first set of updates ran by updateSubview - only being relevant if the default sizes clip off screen.

[jhreis]

Ah, gotcha, I was thinking the height was precalculated, was imaging something like*. But see now why that wasn't possible.

    func updateSubviews() {
        let paddingHeight = padding * 3.0
        let externalContentHeight = dialogButtons.count == 0 ? paddingHeight : kPopupDialogButtonHeight + paddingHeight
        let desiredHeight = UIScreen.main.bounds.height - externalContentHeight

        let resizingRequired = containerView.frame.height > desiredHeight
        let resizePercentage = resizingRequired ? desiredHeight / containerView.frame.height : 1.0
        
        titleLabel.adjustsFontSizeToFitWidth = resizingRequired
        messageLabel.adjustsFontSizeToFitWidth = resizingRequired

        _updateSubviews(resizePercentage)
    }

[jhreis]

So great! Thank you so much for your help!