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

Videojs #743

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from
Open

Videojs #743

wants to merge 3 commits into from

Conversation

Natkeeran
Copy link

! Important - This is for review and feedback.

JIRA Ticket: (link)

What does this Pull Request do?

This PR enables you to add transcript(s) to video/audio objects when using videojs viewer.

What's new?

How should this be tested?

Notes and Questions

Interested parties

@dannylamb @seth-shaw-unlv @DonRichards

@seth-shaw-unlv
Copy link

seth-shaw-unlv commented Nov 7, 2019

A few non-code related notes (I'll add code comments separately):

  • One step missing from the instructions is to enable the islandora_videojs module (e.g. drush en -y islandora_videojs).
  • Even then, you need to view the Media's page to see the captions; the videojs player does not show up on the repository_item page. (I suspect due to how the EVAs work.)
  • Also, and I'm not quite sure where the error is, but part way through my test video the videojs player crashed and did so again after refreshing the page. Now, when I went back to screen-shot the error, it played just fine. (I hate intermittent bugs... especially when they aren't logged anywhere.)
  • The build failed on coding standards. Please use phpcbf.

Copy link

@seth-shaw-unlv seth-shaw-unlv left a comment

Choose a reason for hiding this comment

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

Also need to clean up for coding standards.


$transcript_urls = array();

if ($nid != NULL) {

Choose a reason for hiding this comment

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

You can avoid the Guzzle work by using IslandoraUtils. After you lookup the Term ID, e.g. $transcript_term = getTermForUri('http://pcdm.org/use#Transcript');, take your $node from the routeMatch and use getMediaWithTerm to find your Transcript Media, e.g. $media = getMediaWithTerm($node, $transcript);.

Choose a reason for hiding this comment

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

Between IslandoraUtils and the MediaSourceService you can replace huge chunks of this.

@elizoller
Copy link
Member

@Natkeeran are you still working on this?

@elizoller
Copy link
Member

@Natkeeran I'm keen to see this work make its way into Islandora. Is there anything I can do to help?

@seth-shaw-unlv
Copy link

@elizoller, we may need to consider this PR abandoned. I would probably grab my own branch of Islandora, apply this PR's patch file to your branch, and then address the various comments made on this PR to continue cleaning it up before making a new PR.

@elizoller
Copy link
Member

ok, thanks for the advice @seth-shaw-unlv

@DonRichards
Copy link
Member

Is this still going to be merged?

@elizoller
Copy link
Member

i contributed the work that uses HTML5 video and audio players instead of videojs. i vaguely remember there may have been some community concern about including videojs when it can cost money. but thats a vague memory so if others know something different, please chime in.

@rosiel rosiel changed the base branch from 8.x-1.x to 2.x October 5, 2021 22:28
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.

5 participants