-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
docs(guides): add basic ReactJS integration example #3972
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for the PR. I made a few small comments, once those are all resolved we should be able to get this in.
@@ -269,6 +270,12 @@ Yes! Please [submit an issue or open a pull request][pr-issue-question] if this | |||
|
|||
Yes! Please [submit an issue or open a pull request][pr-issue-question] if this does not work. | |||
|
|||
Use `require('!style-loader!css-loader!video.js/dist/video-js.css')` to inject video.js CSS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add "Be sure to" on the front of this sentence to make it a bit more readable.
import videojs from 'video.js' | ||
|
||
// this loads video.js CSS using webpack loaders | ||
require('!style-loader!css-loader!video.js/dist/video-js.css') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to exclude this line and the comment above as it is more webpack specific
|
||
Here's a basic ReactJS player implementation. | ||
|
||
It just instantiate the video.js player on `componentDidMount` and destroy it on `componentWillUnmount`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instantiateinstantiatesdestroydestroys
} | ||
``` | ||
|
||
You can then use it like this : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the extra space before :
ed52016
to
1802a2c
Compare
Thanks. i added a footnote in react.md about webpack+css. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Can you rebase this? The FAQ was moved back into docs/guides
folder and it's causing a conflict.
export default class VideoPlayer extends React.Component { | ||
componentDidMount() { | ||
// get the <video> DOM node | ||
const videoNode = ReactDOM.findDOMNode(this).querySelector('video'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on <video ref={(c) => { this.videoNode = c; }} />
instead of ReactDOM
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what is "best" way, but you're right, using ref
is more straightforward
|
||
If you use webpack, you can embed video.js CSS like this: | ||
|
||
`require('!style-loader!css-loader!video.js/dist/video-js.css')` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this line more confusing than anything.
If they are using Webpack
, then they already know about loaders
, otherwise they won't get very far.
If they already know about loaders
, then they must also know the power they have to transform files (https://webpack.js.org/guides/code-splitting-css/).
You don't need to have Webpack
to include styles and recommending the inline approach might be less than ideal?
(probably fine in the Webpack
section)
2️⃣ cents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, now that the tooling is stable and mainstream (create-react-app, nwb...) i'm not sure that everyone master webpack loaders config. also the path to the CSS is not obvious so i think its worth mentionning ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for people using browserify or something else, we could also mention alternatives ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just mentioning that you can find the css file at that path and not mention bundler specific stuff?
We also link to it in the package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, removed the mention to webpack 😢
9137a72
to
d4c0ad8
Compare
Now I think the most practical solution is avoiding tooling references and
just mention the problem: "dont forget your css"
…On Jan 25, 2017 19:54, "Gary Katsevman" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In docs/guides/react.md <#3972>:
> +// see https://github.com/videojs/video.js/blob/master/docs/guides/options.md
+const videoJsOptions = {
+ autoPlay: true,
+ controls: true,
+ sources: [{
+ src: '/path/to/video.mp4',
+ type: 'video/mp4'
+ }]
+}
+
+return <VideoPlayer { ...videoJsOptions } />
+```
+
+If you use webpack, you can embed video.js CSS like this:
+
+`require('!style-loader!css-loader!video.js/dist/video-js.css')`
What about just mentioning that you can find the css file at that path and
not mention bundler specific stuff?
We also link to it in the package.json
<https://github.com/videojs/video.js/blob/master/package.json#L6>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3972>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPc5X9h426YIlIkQB_m-va_I4XRi5EKks5rV5o9gaJpZM4Lrhoc>
.
|
@@ -269,6 +270,12 @@ Yes! Please [submit an issue or open a pull request][pr-issue-question] if this | |||
|
|||
Yes! Please [submit an issue or open a pull request][pr-issue-question] if this does not work. | |||
|
|||
Be sure to use `require('!style-loader!css-loader!video.js/dist/video-js.css')` to inject video.js CSS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is tricky and needs more background. First it assumes webpack is being used, but that's the most common case with React anyways. Then style-loader and css-loader are extra packages which must be installed too
It would be better to say that React people should have two concerns:
- Safe DOM manipulation
- Loading Video.js CSS
and that line, is an example of how the solution might be done (in many, but not all cases)
Description
Just some doc for a clean ReactJS integration
Specific Changes proposed
add
guides/react.md
Requirements Checklist