-
Notifications
You must be signed in to change notification settings - Fork 1
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
Make usagePropertiesAutoUpdateMode configurable #25
base: main
Are you sure you want to change the base?
Make usagePropertiesAutoUpdateMode configurable #25
Conversation
@@ -11,6 +11,7 @@ public class ComScoreConfiguration { | |||
public let publisherId: String | |||
public let applicationName: String | |||
public var userConsent: ComScoreUserConsent | |||
public var usagePropertiesAutoUpdateMode: ComscoreUsagePropertiesAutoUpdateMode |
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.
Maybe I don't see the complete picture, but:
if this value can't change (e.g. if we change it, but the underlying analytics plugin is already initialized with the old value (and it never updates)) consider making it a let
, or we need to make sure the underlying code takes into account the updated value
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.
✅
@@ -27,10 +28,11 @@ public class ComScoreConfiguration { | |||
- adIdProcessor: Provide a closure if you want to customize how the ad id is determined. By default, the integration uses Ad.id. |
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.
update the doc pls ;)
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.
✅
@@ -14,6 +14,24 @@ public enum ComScoreUserConsent: String { | |||
case unknown = "-1" | |||
} | |||
|
|||
@frozen | |||
public enum ComscoreUsagePropertiesAutoUpdateMode: String { | |||
case foregroundOnly |
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.
Maybe add a basic doc about these properties. (as this is a public API)
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.
✅
func toComscore() -> SCORUsagePropertiesAutoUpdateMode { | ||
switch self { | ||
case .foregroundOnly: | ||
return .foregroundOnly |
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.
nit-pick: (maybe it is for me personally), but I think in this scenario maybe it would make it a bit more readable, if we use the class name too in the return?
case .foregroundOnly:
return SCORUsagePropertiesAutoUpdateMode.foregroundOnly
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.
✅
Necessary change for react-native-theoplayer-analytics PR for Comscore