Skip to content

MCKIN-12576 Stopped videos while moving to next step - #254

Merged
xitij2000 merged 3 commits into
open-craft:masterfrom
nasirhjafri:MCKIN-12576
Dec 17, 2019
Merged

MCKIN-12576 Stopped videos while moving to next step#254
xitij2000 merged 3 commits into
open-craft:masterfrom
nasirhjafri:MCKIN-12576

Conversation

@nasirhjafri

@nasirhjafri nasirhjafri commented Dec 12, 2019

Copy link
Copy Markdown
Contributor

MCKIN-12576 Stopped videos while moving to the next step

If there is more than one step in assessment and we play a video, the video keeps playing in the background when we move to the next step.

@nasirhjafri

Copy link
Copy Markdown
Contributor Author

@xitij2000 Please review it.

@xitij2000

Copy link
Copy Markdown
Member

@nasirhjafri Sure. I've put it in for review next sprint.

That said I think this approach is too specific and only works for one specific type of video player. I think you should make it more generic so it works with other kinds of video embeds as well. You can do so you can do so by using the underlying HTML video player API. i.e. find all HTML5 video elements and pause them.

@nasirhjafri

Copy link
Copy Markdown
Contributor Author

@xitij2000 Thanks, I've pushed the recommended changes. Please review it in the next sprint.

@xitij2000 xitij2000 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 This is good to merge once the nit has been addressed.

  • I tested this: tested on master devstack
  • I read through the code
  • [na] I checked for accessibility issues
  • [na] Includes documentation

Comment thread problem_builder/public/js/mentoring_with_steps.js Outdated
@xitij2000
xitij2000 merged commit 68c5e38 into open-craft:master Dec 17, 2019
@nasirhjafri
nasirhjafri deleted the MCKIN-12576 branch December 17, 2019 09:17
@nasirhjafri

Copy link
Copy Markdown
Contributor Author

Thanks, @xitij2000 for reviewing.

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.

2 participants