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

Paywall Components Initial Commit #4224

Merged
merged 66 commits into from
Aug 29, 2024
Merged

Conversation

jamesrb1
Copy link
Contributor

@jamesrb1 jamesrb1 commented Aug 26, 2024

Initial components merge. Notes:

  • Linting enabled except for public documentation which will come as the API matures and stabilizes.
  • Took out unused files (tier-related ones), which will come back in a future version. The comments were useful and will be referenced when they come back so thank you for the comments on these.
  • Left in tvOS and watchOS availability checks for now - may revisit this later.
  • Everything is codable.
  • Features added:
    • Layout components allow other components to be laid out along x/y/z axes.
    • Text component supports font weight. (Changing actual font will come later.)
    • LinkButton component links out to external website of choice.
    • RemoteImage allows the image to be arbitrarily styled by the calling code by passing the image back in a closure.
    • Image component supports corner rounding an overlaid gradient. (These will likely be pulled out into common code somewhere in a future PR.)
    • Support for OfferingResponse that includes a separate PaywallComponentsData structure, which is separate from PaywallData used for template-based paywalls.

joshdholtz and others added 30 commits August 17, 2024 14:46
# Conflicts:
#	RevenueCatUI/Data/TemplateViewConfiguration.swift
#	RevenueCatUI/Templates/TemplateViewType.swift
#	Sources/Paywalls/PaywallData.swift
#	Tests/TestingApps/PaywallsTester/PaywallsTester/Config/Configuration.swift
# Conflicts:
#	Tests/TestingApps/PurchaseTesterSwiftUI/Core/ConfiguredPurchases.swift
Removes code we won't be using for some time and splits up some large
files into component files.
Co-authored-by: Josh Holtz <me@joshholtz.com>
…uses on Images

I think we may need a compon properties modifier.
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This is looking really good! I left a bunch of comments (sorry 🙊)

The biggest thing I think is some modification of PaywallData and case templateComponents = "components". Since we added that early checkin PaywallView specifically for components, PaywallData should never anything to do with components anymore.

I might be wrong so please tell me if I am 😇

Comment on lines +232 to +234
if let componentData = offering.paywallComponentsData {
TemplateComponentsView(paywallComponentsData: componentData)
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Reviewer note: this will be reworked in future PR with proper loading and validation ☝️

RevenueCatUI/Templates/TemplateViewType.swift Outdated Show resolved Hide resolved
RevenueCatUI/Views/PurchaseButton.swift Outdated Show resolved Hide resolved
RevenueCatUI/Views/RemoteImage.swift Show resolved Hide resolved
Sources/Paywalls/PaywallData.swift Outdated Show resolved Hide resolved
Sources/Paywalls/PaywallData.swift Outdated Show resolved Hide resolved
Sources/Purchasing/Offering.swift Show resolved Hide resolved
RevenueCatUI/Data/PaywallTemplate.swift Outdated Show resolved Hide resolved
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This looks good to me! I left one question about the version change on Products.storekit but besides that I think this is good 😊

I'd probably like a second pair of eyes looking over this (either @aboedo or somebody else from @RevenueCat/coresdk) but looks likes clean 😎

"major" : 2,
"major" : 3,
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to break anything? Is this because of Xcode 16 or something else? 😅

Copy link
Member

Choose a reason for hiding this comment

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

I think we're good, it's working for me on 15.4.0 (or at least, Xcode doesn't complain but I can't fully test without an API key that has components since otherwise backend response doesn't fully decode)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using Xcode 15, I'm not sure why it made this change, but this is used solely internally for testing, so I don't think there is any major risk here?

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

Looks good for a merge behind feature flag. Only concern would be to move the swiftUI import to behind the flag for the main SDK, so that we're not forcing the import on anyone.
I do think we should find a way to remove the swiftUI dependency from the main SDK completely before shipping components, but that's a ways out anyway

Comment on lines +252 to +257
DebugErrorView(
"\(error.description)\n" +
"You can fix this by editing the paywall in the RevenueCat dashboard.\n" +
"The displayed paywall contains default configuration.\n" +
"This error will be hidden in production.",
replacement: paywallView
Copy link
Member

Choose a reason for hiding this comment

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

[Not for this PR]
I was about to comment that we should move this to one of the strings files, and then I realized that that's part of the main SDK. Maybe we should start thinking about extracting a Core framework to be used by both RevenueCat and RevenueCatUI (we actually used to have one a few years back)

"major" : 2,
"major" : 3,
Copy link
Member

Choose a reason for hiding this comment

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

I think we're good, it's working for me on 15.4.0 (or at least, Xcode doesn't complain but I can't fully test without an API key that has components since otherwise backend response doesn't fully decode)

@joshdholtz
Copy link
Member

Looks good for a merge behind feature flag. Only concern would be to move the swiftUI import to behind the flag for the main SDK, so that we're not forcing the import on anyone. I do think we should find a way to remove the swiftUI dependency from the main SDK completely before shipping components, but that's a ways out anyway

Big +1 on removing SwiftUI from the main SDK 💪 That is happening in a new branch that @jamesrb1 is working on right now 💪

@jamesrb1 Can you move the import SwiftUI behind the flag? And then once you do that... you can merge!

@jamesrb1
Copy link
Contributor Author

@RCGitBot please test

@jamesrb1
Copy link
Contributor Author

I've made changes for most of @aboedo comments (thank you!), and left conversations unresolved for the ones that are not resolved as part of this PR.

@jamesrb1 jamesrb1 merged commit d40eb11 into main Aug 29, 2024
33 checks passed
@jamesrb1 jamesrb1 deleted the paywalls-components/tidied-all-work branch August 29, 2024 00:11
@tonidero tonidero added refactor and removed pr:feat A new feature labels Aug 30, 2024
vegaro pushed a commit that referenced this pull request Aug 30, 2024
**This is an automatic release.**

### Bugfixes
* Fix `compatibleTopBarTrailing` in MacOS and api tests (#4226) via
Cesar de la Vega (@vegaro)
* [Paywall] Fix restoreStarted not being called on
`presentPaywallIfNeeded` when using `requiredEntitlementIdentifier`
(#4223) via Josh Holtz (@joshdholtz)
* [CustomerCenter] Move sheet and restore alert creation to
`ManageSubscriptionsView` (#4220) via Cesar de la Vega (@vegaro)
* [EXTERNAL] `Custom Entitlements Computation`: fix support display on
debug screen (#4215) by @NachoSoto (#4218) via Toni Rico (@tonidero)
* [Customer Center] Add padding to `No thanks` in promotional offer
screen (#4221) via Cesar de la Vega (@vegaro)
* Fix version number in plist files (#4213) via Cesar de la Vega
(@vegaro)
* fix mac os sandbox check slowness (#3879) via Andy Boedo (@aboedo)
* [Customer Center] Fix `FeedbackSurveyView` not opening (#4208) via
Cesar de la Vega (@vegaro)
* Remove `unneeded_override` disable to fix linter (#4209) via Cesar de
la Vega (@vegaro)
### Dependency Updates
* Bump rexml from 3.3.3 to 3.3.6 in
/Tests/InstallationTests/CocoapodsInstallation (#4210) via
dependabot[bot] (@dependabot[bot])
* Bump rexml from 3.3.3 to 3.3.6 (#4211) via dependabot[bot]
(@dependabot[bot])
### Other Changes
* Update readme wording (#3914) via James Borthwick (@jamesrb1)
* Set a maximum duration for iOS 15 tests (#4229) via Cesar de la Vega
(@vegaro)
* Paywall Components Initial Commit (#4224) via James Borthwick
(@jamesrb1)
* [CustomerCenter] Open App Store when the user wants to update their
app (#4199) via JayShortway (@JayShortway)
* [Customer Center] Shows a warning when the app is not the latest
version (#4193) via JayShortway (@JayShortway)
* Fix integration tests simulator version (#4219) via Cesar de la Vega
(@vegaro)
* Pin swift-docc-plugin to 1.3.0 (#4216) via James Borthwick (@jamesrb1)
nyeu pushed a commit that referenced this pull request Oct 2, 2024
Initial components merge. Notes:

- Linting enabled except for public documentation which will come as the
API matures and stabilizes.
- Took out unused files (tier-related ones), which will come back in a
future version. The comments were useful and will be referenced when
they come back so thank you for the comments on these.
- Left in tvOS and watchOS availability checks for now - may revisit
this later.
- Everything is codable.
- Features added:
- Layout components allow other components to be laid out along x/y/z
axes.
- Text component supports font weight. (Changing actual font will come
later.)
  - LinkButton component links out to external website of choice.
- RemoteImage allows the image to be arbitrarily styled by the calling
code by passing the image back in a closure.
- Image component supports corner rounding an overlaid gradient. (These
will likely be pulled out into common code somewhere in a future PR.)
- Support for OfferingResponse that includes a separate
`PaywallComponentsData` structure, which is separate from `PaywallData`
used for template-based paywalls.

---------

Co-authored-by: Josh Holtz <me@joshholtz.com>
nyeu pushed a commit that referenced this pull request Oct 2, 2024
**This is an automatic release.**

### Bugfixes
* Fix `compatibleTopBarTrailing` in MacOS and api tests (#4226) via
Cesar de la Vega (@vegaro)
* [Paywall] Fix restoreStarted not being called on
`presentPaywallIfNeeded` when using `requiredEntitlementIdentifier`
(#4223) via Josh Holtz (@joshdholtz)
* [CustomerCenter] Move sheet and restore alert creation to
`ManageSubscriptionsView` (#4220) via Cesar de la Vega (@vegaro)
* [EXTERNAL] `Custom Entitlements Computation`: fix support display on
debug screen (#4215) by @NachoSoto (#4218) via Toni Rico (@tonidero)
* [Customer Center] Add padding to `No thanks` in promotional offer
screen (#4221) via Cesar de la Vega (@vegaro)
* Fix version number in plist files (#4213) via Cesar de la Vega
(@vegaro)
* fix mac os sandbox check slowness (#3879) via Andy Boedo (@aboedo)
* [Customer Center] Fix `FeedbackSurveyView` not opening (#4208) via
Cesar de la Vega (@vegaro)
* Remove `unneeded_override` disable to fix linter (#4209) via Cesar de
la Vega (@vegaro)
### Dependency Updates
* Bump rexml from 3.3.3 to 3.3.6 in
/Tests/InstallationTests/CocoapodsInstallation (#4210) via
dependabot[bot] (@dependabot[bot])
* Bump rexml from 3.3.3 to 3.3.6 (#4211) via dependabot[bot]
(@dependabot[bot])
### Other Changes
* Update readme wording (#3914) via James Borthwick (@jamesrb1)
* Set a maximum duration for iOS 15 tests (#4229) via Cesar de la Vega
(@vegaro)
* Paywall Components Initial Commit (#4224) via James Borthwick
(@jamesrb1)
* [CustomerCenter] Open App Store when the user wants to update their
app (#4199) via JayShortway (@JayShortway)
* [Customer Center] Shows a warning when the app is not the latest
version (#4193) via JayShortway (@JayShortway)
* Fix integration tests simulator version (#4219) via Cesar de la Vega
(@vegaro)
* Pin swift-docc-plugin to 1.3.0 (#4216) via James Borthwick (@jamesrb1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants