-
Notifications
You must be signed in to change notification settings - Fork 97
Conditional visibility -- app version condition #2894
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
base: jzdesign/conditional-visibility-2
Are you sure you want to change the base?
Conditional visibility -- app version condition #2894
Conversation
Generated by 🚫 Danger |
tonidero
left a comment
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.
Looking good but I do think we need to fix the semver number parsing/comparison
| * */ | ||
| @Serializable | ||
| data class AppVersion( | ||
| @SerialName("operator") val operator: ComparisonOperatorType, |
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.
Should we add in or not_in operators as well? Alternatively, we could add and and or... but maybe in/not_in would be enough.
|
|
||
| /** | ||
| * Serializer that converts version strings like "3.2.0" to integers like 320 by keeping only digits. | ||
| * Also handles integer inputs directly for backward compatibility. |
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.
Hmm does this work for comparison purposes? For example, 11.2.0 would get translated to 1120, and 2.11.0 would get translated to 2110, and it would say that the second is bigger than the first? I think we need proper semver parsing and comparison instead of parsing to an int.
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.
Man… you're right. Thank you for the sanity check here.
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.
Would be good to add some tests for this as well. Using parameterized tests we can cover a lot of cases easily.
Checklist
purchases-iosand hybridsMotivation
As we evolve the conditional visibility feature and add new conditions, our users will likely wind up in a spot where their paywalls don't render in an expected way. We need to provide an option to allow them to account for old SDKs and to control how their paywall renders if they use newer conditions that old SDKs are unaware of.
Description
Adds the ability for users to create a condition that will only show the component for a given app version