Skip to content

Remove autoplay for video in course about page - #2517

Merged
jzoldak merged 1 commit into
openedx:masterfrom
IONISx:remove-video-autoplay
Mar 5, 2014
Merged

Remove autoplay for video in course about page#2517
jzoldak merged 1 commit into
openedx:masterfrom
IONISx:remove-video-autoplay

Conversation

@tusbar

@tusbar tusbar commented Feb 7, 2014

Copy link
Copy Markdown
Contributor

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

tusbar commented Feb 7, 2014

Copy link
Copy Markdown
Contributor Author

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

tusbar commented Feb 12, 2014

Copy link
Copy Markdown
Contributor Author

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

jzoldak commented Feb 27, 2014

Copy link
Copy Markdown
Contributor

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

@cahrens

cahrens commented Mar 3, 2014

Copy link
Copy Markdown

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

tusbar commented Mar 3, 2014

Copy link
Copy Markdown
Contributor Author

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

cahrens commented Mar 3, 2014

Copy link
Copy Markdown

@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

jzoldak commented Mar 3, 2014

Copy link
Copy Markdown
Contributor

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

@nedbat

nedbat commented Mar 4, 2014

Copy link
Copy Markdown
Contributor

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

@jzoldak

jzoldak commented Mar 4, 2014

Copy link
Copy Markdown
Contributor

@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

shnayder commented Mar 4, 2014

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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 <glib.glugovskiy@raccoongang.com>

* 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 <81557788+NiedielnitsevIvan@users.noreply.github.com>
Co-authored-by: Glib Glugovskiy <glib.glugovskiy@raccoongang.com>
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