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

Configuration #4

Merged
merged 2 commits into from
Oct 23, 2015
Merged

Configuration #4

merged 2 commits into from
Oct 23, 2015

Conversation

Fauntleroy
Copy link
Contributor

Objective

Allow configuration of media players via props, specifically the client_id of the SoundCloud player.


What I did

  • Added the soundcloudConfig, vimeoConfig, and youtubeConfig props.
  • Moved some configuration data into defaultProps accordingly.
  • Ensured that every player would automatically set its volume on startup.
  • Updated the debug page.

How to verify

  • Pull this branch
  • Run npm run start
  • Go to http://localhost:3000/
  • Write custom configuration JSON and set it, such as:
{
  "youtube": {
    "playerVars": {
      "autoplay": 0
    }
  }
}
  • When initialized, the YouTube player should be modified accordingly. Note: This will not work on players that are already initialized.
  • Modify the volume
  • Switch to another player
  • The player should be at the volume you set

Screenshots

screen shot 2015-10-18 at 2 12 44 pm

@Fauntleroy
Copy link
Contributor Author

One important thing to note is that if you pass in a configuration object via props it will completely replace the default one. If preserving the other defaults while changing one thing is important to you, I can try to merge the configuration objects instead.

@@ -38,6 +38,15 @@ export default class App extends Component {
this.setState(state)
}
}
onConfigSubmit = () => {
var config
Copy link
Owner

Choose a reason for hiding this comment

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

You should use let here I think

@cookpete
Copy link
Owner

Hi @Fauntleroy. Many thanks for your contribution!

I'm wondering if a prop for a config for each player is polluting the top level props too much, and maybe something like a playerConfig object to hold all of them would be better? Although then I suppose it could get awkward with default props and object merging for each player.

I also wonder if the deep structure is slightly overkill at this point, and whether we should just use youtubePlayerVars, soundcloudClientId and vimeoFrameParams instead.

@cookpete
Copy link
Owner

Also, I've just pushed 0.1.0 with file support and some other minor bits which you will have to rebase on. Apologies!

@Fauntleroy
Copy link
Contributor Author

No problem, @cookpete! I'm more than happy to contribute to anything that'll rid me of JWPlayer (and the delicate browserify-shims that come along with it).

Concerning the use of top-level props like soundcloud, vimeo, and youtube, I spent a while thinking about it last weekend and chose this approach for the following reasons:

  • Why props? Using props to configure a react component seems like the best way to handle this (as opposed to a static like Player.setConfig(...))
  • Why top-level? These are pretty obscure configuration options that I don't expect most users of this component to utilize (aside from maybe the SoundCloud client_id, which you might want to emphasize). Since these props are optional, that means most users won't ever have to pollute their render methods with them.
  • Why not one playerConfig object? Using separate props with keys related to each provider, in my opinion, is a cleaner and easier to use abstraction in practice.
  • Why deep objects? These objects, while a bit unweildy, leave plenty of room for adding additional configuration options in the future. I'm going to resolve the issues with preserving react-player defaults by using Object.assign (or similar) to merge those defaults properly when the SoundCloud, Vimeo, or YouTube player is initialized.

@Fauntleroy
Copy link
Contributor Author

I responded to the code review comments, updated the way default playerVars and iframeParams are handled, and fixed the initial volume issue with the new FilePlayer.

@@ -6,9 +7,20 @@ import Base from './Base'
const IFRAME_SRC = 'https://player.vimeo.com/video/'
const MATCH_URL = /https?:\/\/(?:www\.|player\.)?vimeo.com\/(?:channels\/(?:\w+\/)?|groups\/([^\/]*)\/videos\/|album\/(\d+)\/video\/|video\/|)(\d+)(?:$|\/|\?)/
const MATCH_MESSAGE_ORIGIN = /^https?:\/\/player.vimeo.com/
const DEFAULT_IFRAMEPARAMS = {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we call this DEFAULT_IFRAME_PARAMS?

@cookpete
Copy link
Owner

Typo: iframParams in commit message for 989d4f1

@cookpete
Copy link
Owner

Looking good Tim. I've made a few more notes, then we should be more or less good to merge :)

@Fauntleroy
Copy link
Contributor Author

Variable names changed, commit message amended.

@cookpete
Copy link
Owner

Nice work. Could you squash the commits up into the different features you added? I think it could just be one for the config stuff, and one for the volume setting fixes. Leaving in fixup commits feels a bit untidy.

@Fauntleroy
Copy link
Contributor Author

Commits squashed.

cookpete added a commit that referenced this pull request Oct 23, 2015
@cookpete cookpete merged commit 9fc2d9e into cookpete:master Oct 23, 2015
cookpete added a commit that referenced this pull request Nov 6, 2015
From #4
david-hub024 pushed a commit to david-hub024/React_VideoPlayer that referenced this pull request Dec 23, 2018
@mgw-sbex mgw-sbex mentioned this pull request Jan 25, 2019
david-hub024 pushed a commit to david-hub024/React_VideoPlayer that referenced this pull request May 23, 2020
albanqoku added a commit to albanqoku/react-player that referenced this pull request Feb 24, 2021
Webmaster1116 added a commit to Webmaster1116/video-player that referenced this pull request May 20, 2021
webmiraclepro added a commit to webmiraclepro/video-player that referenced this pull request Sep 9, 2022
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.

None yet

2 participants