Skip to content

Remove "show_closed_captions" in cookie - #13798

Closed
myleshk wants to merge 2 commits into
open-release/eucalyptus.masterfrom
unknown repository
Closed

Remove "show_closed_captions" in cookie#13798
myleshk wants to merge 2 commits into
open-release/eucalyptus.masterfrom
unknown repository

Conversation

@myleshk

@myleshk myleshk commented Oct 20, 2016

Copy link
Copy Markdown

Storing caption status in global cookie is wired. Below are my reasons:

  1. This cookie is only useful when you watch the same video once and refresh/close the page and then come back to watch again, which does not make much sense.
  2. Current implementation of this cookie is global, and will override/interfere with other videos in the same domain (not only same course).
  3. Because of 2, if the other video does not have transcripts(captions), the toggle button of captions will not show up, so there is no way of closing the captions even during video playing. See below.

Image of #3

Please check this problem. Thx.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @myleshk! I've created OSPR-1519 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Oct 20, 2016
@gsong

gsong commented Oct 21, 2016

Copy link
Copy Markdown
Contributor

jenkins run all

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed needs triage labels Oct 21, 2016
@myleshk

myleshk commented Oct 25, 2016

Copy link
Copy Markdown
Author

Just made some new changes that should fix relevant test failures.

Not sure for other test failures about "problems" as I just changed one JavaScript file of video player.

Please run tests again, thank you.

@shaunagm

Copy link
Copy Markdown
Contributor

jenkins run all

@cptvitamin

Copy link
Copy Markdown
Contributor

@myleshk I believe #2 is the desired behavior. If you are hearing impaired for one course, you are likely hearing impaired for all courses.

WRT #3, all videos should have captions defined in Studio.

@myleshk

myleshk commented Nov 28, 2016

Copy link
Copy Markdown
Author

I'm closing this because the major problem this PR tries to solve is cause by my ignorance of edX MDC.

However, for Problem 2 I still think it is bothering ordinary users with occasional needs to use captions, e.g. due to noisy environment, unfamiliar terms in one video.

@myleshk myleshk closed this Nov 28, 2016
@myleshk
myleshk deleted the open-release/eucalyptus.master branch December 2, 2016 05:49
@myleshk
myleshk restored the open-release/eucalyptus.master branch December 2, 2016 05:49
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 product review PR requires product review before merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants