Conversation
🦋 Changeset detectedLatest commit: 98fa575 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 |
| enter_fullscreen = 10 | ||
| exit_fullscreen = 11 | ||
| view = 12 | ||
| } |
There was a problem hiding this comment.
Just out of curiousity, is there a reason to not include some of the available events (eg skip and cued
There was a problem hiding this comment.
I've noticed those around, but I'm not sure what their intention is. Are these things we want to track?
They are not currently part of the Ophan model: https://dashboard.ophan.co.uk/docs/thrift/media.html#Enum_MediaEvent
There was a problem hiding this comment.
That's interesting. It looks like they were introduced to the thrift model over 2 years ago so not really in the scope of this PR anyway. As far as I'm aware, we're not including a skip button in the upcoming mvp player release so these are events we need to worry about for now.
There was a problem hiding this comment.
Good find! There is some missing context but I think that PR was attempting to codify the types already being used: https://github.com/guardian/ophan/pull/5822/changes#diff-4a7da8dbfd99c582271f26a2242b40557a69b3165b08bab3bf534c91e0c9064bR40-R44
The thrift model is a separate entity to those types and has no reference to those types. The conversion between what the web provides and the Ophan model expects takes place here: https://github.com/guardian/ophan/blob/258357082296624776c2bcc3ec8375779349b063/the-slab/app/extractors/MediaPlaybackExtractor.scala#L15-L20
abeddow91
left a comment
There was a problem hiding this comment.
Checked against https://github.com/guardian/ophan/pull/8060/changes and this looks to include all of the new events.
What does this change?
Updates the
MediaEventthrift definition to match the new Ophan events. These correspond to tracking events we would like to send for self-hosted video.