Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paywalls: new PaywallViewControllerDelegate.paywallViewController(_:didChangeSizeTo:) #3563

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented Jan 9, 2024

  • Add paywallViewControlleSizeDidChange to notify of size changes
  • Added back top anchor to footers for auto layout to size correctly

Copy link
Contributor

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Just a couple of comments from our initial prototype implementations.
Also this needs to go into the API tester: https://github.com/RevenueCat/purchases-ios/blob/main/Tests/APITesters/RevenueCatUIAPITester/SwiftAPITester/PaywallViewControllerAPI.swift#L31

Comment on lines 139 to 140
@objc(paywallViewControlleSizeDidChange:)
optional func paywallViewControlleSizeDidChange(_ size: CGSize)
Copy link
Contributor

@NachoSoto NachoSoto Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a doc to this and the sender controller like the others have:

Suggested change
@objc(paywallViewControlleSizeDidChange:)
optional func paywallViewControlleSizeDidChange(_ size: CGSize)
/// For internal use only.
@objc(paywallViewController:didChangeSize:)
optional func paywallViewController(_ controller: PaywallViewController, didChangeSize size: CGSize)

Comment on lines 75 to 78
.onSizeChange { [weak self] in
self?.delegate?.paywallViewControlleSizeDidChange?($0)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this becomes:

Suggested change
.onSizeChange { [weak self] in
self?.delegate?.paywallViewControlleSizeDidChange?($0)
}
.onSizeChange { [weak self] in
guard let self = self else { return }
self.delegate?.paywallViewController(self, didChangeSize: $0)
}

Copy link
Contributor Author

@vegaro vegaro Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be didChangeSizeWith?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe didChangeSizeTo?

@NachoSoto NachoSoto added refactor A code change that neither fixes a bug nor adds a feature and removed RevenueCatUI labels Jan 9, 2024
@NachoSoto NachoSoto changed the title Add paywallViewControlleSizeDidChange Paywalls: new PaywallViewControllerDelegate.paywallViewController(_:didChangeSize:) method Jan 9, 2024
@NachoSoto
Copy link
Contributor

Changed the label to refactor since this is meant for internal use only.

@NachoSoto NachoSoto changed the title Paywalls: new PaywallViewControllerDelegate.paywallViewController(_:didChangeSize:) method Paywalls: new PaywallViewControllerDelegate.paywallViewController(_:didChangeSize:) Jan 9, 2024
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! After Nacho's comments are fixed will rereview.

@NachoSoto NachoSoto changed the title Paywalls: new PaywallViewControllerDelegate.paywallViewController(_:didChangeSize:) Paywalls: new PaywallViewControllerDelegate.paywallViewController(_:didChangeSizeTo:) Jan 10, 2024
@vegaro vegaro merged commit 280964f into main Jan 10, 2024
24 checks passed
@vegaro vegaro deleted the onSizeChange branch January 10, 2024 15:10
NachoSoto pushed a commit that referenced this pull request Jan 10, 2024
**This is an automatic release.**

### RevenueCatUI
* `Paywalls`: remove unscrollable spacing in Template 5 (#3562) via
NachoSoto (@NachoSoto)
* `Paywalls`: improve template 5 checkmark icon (#3559) via NachoSoto
(@NachoSoto)
### Bugfixes
* Improve sandbox detector for macOS apps (#3549) via Mark Villacampa
(@MarkVillacampa)
### Other Changes
* `Paywalls`: new
`PaywallViewControllerDelegate.paywallViewController(_:didChangeSizeTo:)`
(#3563) via Cesar de la Vega (@vegaro)
* `Tests`: running tests on `macOS` (#3533) via NachoSoto (@NachoSoto)
* `Integration Tests`: split into separate jobs (#3560) via NachoSoto
(@NachoSoto)
NachoSoto added a commit to RevenueCat/purchases-hybrid-common that referenced this pull request Jan 10, 2024
Depends on RevenueCat/purchases-ios#3563

---------

Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>
NachoSoto added a commit that referenced this pull request Jan 11, 2024
NachoSoto added a commit that referenced this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants