Skip to content
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

#2128 trigger pause when media component leaves view #194

Merged
merged 5 commits into from
Apr 26, 2019

Conversation

jamesrea83
Copy link
Member

In response to #2128.

As per this comment media component is paused directly, rather than triggering media:stop.

@moloko
Copy link
Contributor

moloko commented Apr 8, 2019

I think this behaviour should be configurable via a property. Question is: should it be enabled by default or not? Thoughts?

@brian-learningpool
Copy link
Member

I agree this setting should be configurable via a property.

I was initially going to say defaulted to on, but I've changed my mind and gone with a default of off for two reasons:

  • backwards compatibility (maintaining existing functionality)
  • the Media component also handles playing MP3 files, where it's probably acceptable to continue playing while not in view

@moloko
Copy link
Contributor

moloko commented Apr 8, 2019

fine by me, tbh it’s not really an issue in the framework where this sort of thing is easily changed via find-and-replace, it’s more an issue for authoring tool users.

README.md Outdated Show resolved Hide resolved
bower.json Outdated Show resolved Hide resolved
@jamesrea83
Copy link
Member Author

@oliverfoster what do you think about attaching the listener only when playback starts, and removing it when it stops? As opposed to the current adding it to setupEventListeners / remove.

@oliverfoster
Copy link
Member

oliverfoster commented Apr 9, 2019

@oliverfoster what do you think about attaching the listener only when playback starts, and removing it when it stops? As opposed to the current adding it to setupEventListeners / remove.

It would be preferable from a CPU usage perspective to attach the inview listeners only when they are needed but could also make for more sprawling code. Having good names and layouts of the functions would mitigate the secondary layer of event attachments / removals and you will still need to remove the event attachments when the component is destroyed.

Really, it's only a CPU problem if you have loads of things listening for inview at the same time. It might be worth leaving the code as it is, it's nice and simple.

Copy link
Member

@danielstorey danielstorey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Couple of really minor comments

js/adapt-contrib-media.js Show resolved Hide resolved
js/adapt-contrib-media.js Outdated Show resolved Hide resolved
Copy link
Contributor

@moloko moloko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code 👁

js/adapt-contrib-media.js Outdated Show resolved Hide resolved
@moloko moloko merged commit 16cdec4 into adaptlearning:master Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants