Conversation
ryanjduffy
left a comment
ryanjduffy
left a comment
There was a problem hiding this comment.
Thanks, @teckliew! I think the HOC approach is good and I like how this is shaping up. Couple considerations below. Let me know what you think!
| render: (props) => ( | ||
| <Panel {...props}> | ||
| <Header title="VideoPlayer" titleBelow="powered by Shaka Player" /> | ||
| <ShakaVideoPlayer autoPlay manifestUri={sintelManifestUri} /> |
There was a problem hiding this comment.
autoPlay doesn't seem to be working. Are you seeing the same?
| onError(event.detail); | ||
| } | ||
|
|
||
| function onError(error) { |
There was a problem hiding this comment.
Let's make this a bit more React-y by moving these handlers into the component and invoking callbacks via props as usual. Could then move the console.error into the app instead of here.
|
|
||
| render () { | ||
| const props = Object.assign({}, this.props); | ||
| delete props.manifestUri; |
|
|
||
| function initPlayer(config, manifestUri) { | ||
| // Create a Player instance. | ||
| const video = document.querySelector('video'); |
There was a problem hiding this comment.
I discovered the issue with getVideoNode(). We abstracted access to the DOM node behind ui/Media so you have to do something like videoPlayer.getVideoNode().media to get all the way to the node.
I'm not sure we should expose that (it's not terrible but it isn't good either) but I think a "within React" impl (e.g. using findDOMNode() or the above code) would be better to show in a sample than querySelector. Wdyt?
| console.error('Error code', error.code, 'object', error); | ||
| } | ||
|
|
||
| const defaultConfig = { |
There was a problem hiding this comment.
Should these be runtime props vs design time configs?
Signed-off-by: Ryan Duffy <ryan.duffy@lge.com>
Signed-off-by: Ryan Duffy <ryan.duffy@lge.com>
ryanjduffy
left a comment
ryanjduffy
left a comment
There was a problem hiding this comment.
I made a couple small changes (forward shaka errors out, rename HOC for consistency and export it) to put it more inline with our conventions for pattern samples. Seems like a great starting point for using Shaka Player. Thanks, @teckliew!
webOS101
left a comment
webOS101
left a comment
There was a problem hiding this comment.
If we just needed a demo and aren't going to publish this, we don't need to really address these comments. Leaving them here in case we want to do more with it.
| { | ||
| "name": "shaka-app", | ||
| "version": "1.0.0", | ||
| "description": "A general template for an Enact Moonstone application.", |
There was a problem hiding this comment.
Let's update the description for completeness
| "name": "shaka-app", | ||
| "version": "1.0.0", | ||
| "description": "A general template for an Enact Moonstone application.", | ||
| "author": "", |
There was a problem hiding this comment.
Want to add your name here, Teck?
| "test": "enact test", | ||
| "test-watch": "enact test --watch" | ||
| }, | ||
| "license": "UNLICENSED", |
There was a problem hiding this comment.
Let's make this apache like our others.
| "dist/*" | ||
| ], | ||
| "dependencies": { | ||
| "@enact/core": "^2.0.0", |
There was a problem hiding this comment.
Should we update to 3.x? Add ilib? Update React?
| this.onError(event.detail); | ||
| } | ||
|
|
||
| onError = handle( |
There was a problem hiding this comment.
Is this the right way to bind the handler?
|
Teck Liew seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
4 participants
Add a video player sample that uses Shaka: https://shaka-player-demo.appspot.com/docs/api/index.html