Skip to content
This repository was archived by the owner on Jan 5, 2019. It is now read-only.

[WIP] Implementing the MediaPlayer, Audio, and Video Components#149

Open
jrgriffiniii wants to merge 2 commits intoLafayetteCollegeLibraries:masterfrom
jrgriffiniii:issues-105-plyr-griffinj
Open

[WIP] Implementing the MediaPlayer, Audio, and Video Components#149
jrgriffiniii wants to merge 2 commits intoLafayetteCollegeLibraries:masterfrom
jrgriffiniii:issues-105-plyr-griffinj

Conversation

@jrgriffiniii
Copy link
Copy Markdown
Contributor

Resolves #105

@jrgriffiniii
Copy link
Copy Markdown
Contributor Author

jrgriffiniii commented Apr 4, 2017

The current state of this PR is 👎 (there is definitely refactoring and abstraction needed) , but I would greatly appreciate any feedback and advice before I proceed further.

@coveralls
Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 05b23db on jrgriffiniii:issues-105-plyr-griffinj into ** on LafayetteCollegeLibraries:master**.

@jrgriffiniii
Copy link
Copy Markdown
Contributor Author

Commit 05b23db integrates the MediaPlayer Component with the Work interface. For the purposes of capturing feedback, it is (purposely) set as the default viewer for all Work resources.

Copy link
Copy Markdown
Member

@rococodogs rococodogs left a comment

Choose a reason for hiding this comment

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

👍 looks very good to me! can confirm that the video plays when the viewer is opened

@rococodogs
Copy link
Copy Markdown
Member

If I have any suggestions for refactoring so far, it would be to maybe make the <Audio>/<Video> components into functional ones:

function Audio (props) {
  const { sources, ...audioProps } = props
  return (
    <audio {...audioProps}>
      { (sources || []).map((s, idx) => <source {...s} key={idx} />) }
    </audio>
  )
}

function Video (props) {
  const { sources, captions, ...videoProps } = props
  return (
    <video {...videoProps}>
      { (sources || []).map((s, idx) => <source {...s} key={idx} />) }
      { captions ? <track {...captions} key="captions" /> : '' }
    </video>
  ) 
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants