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

[SE-4475] Add transcripts download and refactor captions download #11

Merged

Conversation

gabor-boros
Copy link
Contributor

@gabor-boros gabor-boros commented Jun 2, 2021

The "download transcripts" button already exists, though it actually downloads the captions, not transcripts. Hence that button -- and the related code -- is renamed to use the term "captions". Because the original intention was to add a download transcripts button in #6, this commit adds the originally required button and functionality as well.

Besides that, it worth mentioning the transcripts are different in a way compared to captions in a way that it is not bound to any language.

Dependencies: None

Screenshots:

Only captions:
Screenshot 2021-06-02 at 16 45 42

Only transcripts:
Screenshot 2021-06-02 at 16 58 24

Both transcripts and captions:
Screenshot 2021-06-02 at 16 44 54

Sandbox URL: Sandbox URL
Merge deadline: None

Testing instructions:

  • Open the URL and check the 3 videos has a) no button b) only transcripts c) both captions and transcripts. (only captions not shown but up on request, I can show it as well)

--- OR ---

  1. Start Studio and LMS
  2. Upload a video with transcript (any txt file is sufficient)
  3. Setup the XBlock as described in the README
  4. View the video in LMS
  5. Validate that the transcripts opens in a new blank page

Author notes and concerns:

Wistia using caching for the transcripts, therefore, in case of a transcript is changed or removed, it may take some time to be available/unavailable.

Reviewers

pkulkark
pkulkark previously approved these changes Jun 10, 2021
Copy link

@pkulkark pkulkark left a comment

Choose a reason for hiding this comment

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

LGTM 👍

  • I tested this: verified the download captions and download transcripts work as described in testing instuctions
  • I read through the code

@gabor-boros gabor-boros marked this pull request as ready for review June 10, 2021 07:27
@gabor-boros
Copy link
Contributor Author

Hello @OmarIthawi,
May I request a review for this PR?

@OmarIthawi
Copy link
Contributor

@gabor-boros let's please wait till #15 is merged. Then please rebase this pull request to run tests on GitHub.

@OmarIthawi
Copy link
Contributor

@gabor-boros please rebase over #15 so we can run tests on GitHub Actions.

@gabor-boros
Copy link
Contributor Author

Hello @OmarIthawi,
I'll do today or latest on Monday -- I may need to include another change as well and if possible I would like to handle that in one PR 😇

The "download transcripts" button already exists, though it actually
downloads the captions not transcripts. Hence that button -- and the
related code -- is renamed to use the term "captions". Because the
original intention was to add a download transcripts button in appsembler#6, this
commit adds the originally required button and functionality as well.

Besides that, it worth mentioning the transcripts are different in a way
compared to captions in a way that it is not bind to any language.
@gabor-boros
Copy link
Contributor Author

gabor-boros commented Jun 11, 2021

@OmarIthawi the PR is rebased now. 🤞 -- no further changes will come for now

@OmarIthawi OmarIthawi merged commit 86fcd84 into appsembler:master Jun 12, 2021
OmarIthawi added a commit that referenced this pull request Jun 12, 2021
Release #11 in addition to few package upgrades.
@OmarIthawi OmarIthawi mentioned this pull request Jun 12, 2021
@OmarIthawi
Copy link
Contributor

@gabor-boros gabor-boros deleted the gabor/add-transcripts-download branch June 14, 2021 13:51
@gabor-boros
Copy link
Contributor Author

@OmarIthawi didn't you miss v as a prefix for the release tag? (other releases has the v prefix)

@OmarIthawi
Copy link
Contributor

Fixed that. Thanks @gabor-boros!

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.

3 participants