-
Notifications
You must be signed in to change notification settings - Fork 3
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
Bugfix/comscore improvements #27
Conversation
🦋 Changeset detectedLatest commit: 955b853 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
9582bfe
to
eab424d
Compare
comscore/README.md
Outdated
// Execute a function with platform-specific code to retrieve up-to-date information. | ||
runPlatformSpecificCodeToRetrieveValues(onSuccessCallback, onErrorCallback); | ||
} | ||
// |
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.
Empty comment can be removed
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.
✅
console.log( | ||
`[COMSCORE - StreamingAnalytics] startFromDvrWindowOffset ${this.dvrWindowOffsetMs}` | ||
); | ||
} |
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.
Can maybe include this in a more general setContentMetadata
and/or make a separate function for it since it is duplicated below as well?
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.
Decided to leave it out of setContentMetadata
(since it only prepares the metadata object and interacts with only one StreamingAnalyics api) but extracted it to a separate function
private isBeforePreRoll = (): boolean => | ||
this.player.ads?.scheduledAdBreaks.length ? this.player.ads?.scheduledAdBreaks[0].timeOffset === 0 : false; | ||
private isBeforePreRoll = (): boolean => { | ||
const hasScheduledAdBreaks = this.player.ads?.scheduledAdBreaks.length !== 0; |
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.
Can player.ads
ever be undefined here? Might want to add an early return if that's the case?
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.
✅
A variety of improvements to the comscore connector: