-
Notifications
You must be signed in to change notification settings - Fork 4k
✨ Add firstPlay
event
#15074
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
✨ Add firstPlay
event
#15074
Conversation
src/service/video-manager-impl.js
Outdated
@@ -430,6 +430,20 @@ class VideoEntry { | |||
|
|||
element.signals().whenSignal(VideoEvents.REGISTERED) | |||
.then(() => this.onRegister_()); | |||
|
|||
// |
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.
rm
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.
Done.
src/service/video-manager-impl.js
Outdated
* Trigger event for first manual play. | ||
* @private @const {!function()} | ||
*/ | ||
this.triggerFirstPlayOrNoop_ = once(() => { |
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.
let's make the naming and location of the method match existing timeUpdateActionEvent_
method.
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.
The event is fired per-video, so it should be at the entry level. Unless you mean firstPlay
should be triggered once per document?
src/service/video-manager-impl.js
Outdated
* Trigger event for first manual play. | ||
* @private @const {!function()} | ||
*/ | ||
this.triggerFirstPlayOrNoop_ = once(() => { |
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.
🙇 for adding once
to our library, so useful.
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.
It's been handy once
or twice
before 🤓
src/service/video-manager-impl.js
Outdated
*/ | ||
this.triggerFirstPlayOrNoop_ = once(() => { | ||
const trust = ActionTrust.LOW; | ||
const event = createCustomEvent(this.ampdoc_.win, 'firstPlay', |
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.
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.
Done.
src/service/video-manager-impl.js
Outdated
const trust = ActionTrust.LOW; | ||
const event = createCustomEvent(this.ampdoc_.win, 'firstPlay', | ||
/* detail */ {}); | ||
const actions = Services.actionServiceForDoc(this.ampdoc_); |
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.actions_
should be available.
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.
It's available at the manager level, see comment above.
src/service/video-manager-impl.js
Outdated
*/ | ||
this.triggerFirstPlayOrNoop_ = once(() => { | ||
const trust = ActionTrust.LOW; | ||
const event = createCustomEvent(this.ampdoc_.win, 'firstPlay', |
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.
const for firstPlay
(mostly yo make it consistent with timeUpdateActionEvent_
)
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.
Done.
No description provided.