-
Notifications
You must be signed in to change notification settings - Fork 333
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 V2] New overrides structure #4705
Conversation
1 build decreased size
Paywalls 1.0 (1)
|
Item | Install Size Change |
---|---|
DYLD.String Table | ⬇️ -39.9 kB |
RevenueCatUI.TextComponentViewModel.TextComponentViewModel | ⬇️ -14.4 kB |
RevenueCatUI.ImageComponentViewModel.ImageComponentViewModel | ⬇️ -12.2 kB |
Code Signature | ⬇️ -4.9 kB |
DYLD.Exports | ⬇️ -1.4 kB |
🛸 Powered by Emerge Tools
Comment trigger: Size diff threshold of 100.00kB exceeded
for presentedOverride in presentedOverrides where self.shouldApply( | ||
for: presentedOverride.conditions, | ||
state: state, | ||
activeCondition: condition, | ||
isEligibleForIntroOffer: isEligibleForIntroOffer) { | ||
presentedPartial = Self.combine(presentedPartial, with: presentedOverride.properties) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the overrides are iterated from lowest to higher priority (in the order they are given) and will only combine them if all the conditions are true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean!
private static func shouldApply( | ||
for conditions: [PaywallComponent.Condition], | ||
state: ComponentViewState, | ||
activeCondition: ScreenCondition, | ||
isEligibleForIntroOffer: Bool | ||
) -> Bool { | ||
// Early return when any condition evaluates to false | ||
for condition in conditions { | ||
switch condition { | ||
case .compact, .medium, .expanded: | ||
if !activeCondition.applicableConditions.contains(condition) { | ||
return false | ||
} | ||
case .introOffer: | ||
if !isEligibleForIntroOffer { | ||
return false | ||
} | ||
case .selected: | ||
if state != .selected { | ||
return false | ||
} | ||
case .unsupported: | ||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic for determining if all the conditions are true
// For unknown cases | ||
case unsupported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a case for unsupported
so that the decoding wouldn't fail for multiple_intro_offers
or if any other new ones where added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! 🙌
for presentedOverride in presentedOverrides where self.shouldApply( | ||
for: presentedOverride.conditions, | ||
state: state, | ||
activeCondition: condition, | ||
isEligibleForIntroOffer: isEligibleForIntroOffer) { | ||
presentedPartial = Self.combine(presentedPartial, with: presentedOverride.properties) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean!
} | ||
|
||
return true | ||
} | ||
|
||
} | ||
|
||
private extension ScreenCondition { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not suggesting or requesting any changes, especially not in this PR, but do we still need ScreenCondition
? Can we not make do with just PaywallComponent.Condition
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhhhhh, this is a good point! I think we can totally merge the two in a follow up PR 👍
// For unknown cases | ||
case unsupported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
Motivation
Migrating
overrides
from a structured object to an array of override objectsDescription
Previously
Overrides were a very structure object that had all of the different types of states/conditions that allowed base properties of a component to be overridden. However, this was not flexible when it came to multiple being activated as it was missing for cases when multiples types of conditions were being evaluated.
Now
Overrides is an array that has an array of
conditions
andproperties
. It will get applied in order from first to last IF all of theconditions
are true.This allows the FE/BE to determine the orders of overrides from lowest to higher priority so all of the SDKs/renderers don't need to duplicate that logic.