Skip to content

Remove autoplay for video in course about page #2517

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

Merged
merged 1 commit into from
Mar 5, 2014

Conversation

tusbar
Copy link
Contributor

@tusbar tusbar commented Feb 7, 2014

The iframe was generated with autoplay=1, which, sometimes, would cause the video to play in the background.

This is a quick fix, it won’t fix the existing courses as the <iframe> is stored in the database. To do so, you’ll have to edit the "about/video.html" file manually.

The iframe was generated with autoplay=1, which, sometimes, would cause
the video to play in the background.
@tusbar
Copy link
Contributor Author

tusbar commented Feb 7, 2014

Also, autoplay=1 is appended by https://github.com/edx/edx-platform/blob/master/lms/static/js/toggle_login_modal.js#L105 when opening the modal.

@tusbar
Copy link
Contributor Author

tusbar commented Feb 12, 2014

Actually, there’s no need to manually edit the about/video.hml files, resubmitting anything on the “Schedule and Details” settings page in Studio will regenerate it.

@jzoldak
Copy link
Contributor

jzoldak commented Feb 27, 2014

@cahrens should we get a review from the studio team?

@cahrens
Copy link

cahrens commented Mar 3, 2014

I'm unsure what the desired change in behavior is. Is it to stop autoplay of the course about video just when editing the course in Studio, or is it to stop autoplay in the LMS? If it is LMS, doesn't the fact that autoplay=1 is appended in toggle_login_modal mean that this change is not relevant?

@tusbar
Copy link
Contributor Author

tusbar commented Mar 3, 2014

Course about page, in the LMS.

The JavaScript I linked does not append autoplay=1 to the iframe’s src attribute, but to the data-src attribute where it’s being copied to.

The src attribute is then cleared out (L106).

This pretty much fixes a race condition that would occur when the JavaScript would take some time to load/run and the video would start playing in the background.

@cahrens
Copy link

cahrens commented Mar 3, 2014

@jzoldak I think an LMS review would be sufficient. They better understand the behavior they want to achieve. And a test would be on the LMS side.

@jzoldak
Copy link
Contributor

jzoldak commented Mar 3, 2014

@shnayder @nedbat can this get on the list for lms team review.

@nedbat
Copy link
Contributor

nedbat commented Mar 4, 2014

@jzoldak I'm the wrong person to ask about the LMS team these days. @dianakhuang?

@jzoldak
Copy link
Contributor

jzoldak commented Mar 4, 2014

@nedbat I was actually tagging you from an opensource perspective. I'm not sure this is the way edx.org wants it to work, although it might be the way that people in the openedx community want it to? Or maybe I don't understand the change.
Actually thinking further about this, it would not affect edx.org which is fronted by drupal for the course about pages. But I'm pretty sure it would affect edge.

@shnayder
Copy link

shnayder commented Mar 4, 2014

I'm happy with the change in behavior. @dianakhuang can you take a quick look at the code (it's one line)?

@dianakhuang
Copy link
Contributor

Looks good to me. 👍

jzoldak pushed a commit that referenced this pull request Mar 5, 2014
…play

Remove autoplay for video in course about page
@jzoldak jzoldak merged commit ad714a1 into openedx:master Mar 5, 2014
@dangtrinhnt
Copy link

Thanks @tusbar !

@tusbar tusbar deleted the remove-video-autoplay branch June 1, 2014 15:03
cmltaWt0 pushed a commit that referenced this pull request Nov 1, 2024
* feat: [AXM-24] Update structure for course enrollments API (#2515)

* feat: [AXM-24] Update structure for course enrollments API

* style: [AXM-24] Improve code style

* fix: [AXM-24] Fix student's latest enrollment filter

* feat: [AXM-47] Add course_status field to primary object (#2517)

* feat: [AXM-40] add courses progress to enrollment endpoint (#2519)

* fix: workaround for staticcollection introduced in e40a01c

* feat: [AXM-40] add courses progress to enrollment endpoint

* refactor: [AXM-40] add caching to improve performance

* refactor: [AXM-40] add progress only for primary course

* refactor: [AXM-40] refactor enrollment caching optimization

---------

Co-authored-by: Glib Glugovskiy <[email protected]>

* feat: [AXM-53] add assertions for primary course (#2522)

* feat: [AXM-53] add assertions for primary course

* test: [AXM-53] fix tests

* style: [AXM-53] change future_assignment default value to None

* refactor: [AXM-53] add some optimization for assignments collecting

* feat: [AXM-200] Implement user's enrolments status API

* style: [AXM-200] Improve code style

* refactor: [AXM-200] Divide get method into smaller methods

---------

Co-authored-by: NiedielnitsevIvan <[email protected]>
Co-authored-by: Glib Glugovskiy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants