docs(guides): add basic ReactJS integration example #3972

revolunet · 2017-01-23T20:47:53Z

Description

Just some doc for a clean ReactJS integration

Specific Changes proposed

add guides/react.md

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

brandonocasey

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.

brandonocasey · 2017-01-23T21:06:17Z

docs/faq.md

@@ -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.


I would add "Be sure to" on the front of this sentence to make it a bit more readable.

brandonocasey · 2017-01-23T21:15:53Z

docs/guides/react.md

+import videojs from 'video.js'
+
+// this loads video.js CSS using webpack loaders
+require('!style-loader!css-loader!video.js/dist/video-js.css')


I think we may want to exclude this line and the comment above as it is more webpack specific

brandonocasey · 2017-01-23T21:15:54Z

docs/guides/react.md

+
+Here's a basic ReactJS player implementation.
+
+It just instantiate the video.js player on `componentDidMount` and destroy it on `componentWillUnmount`.


~~instantiate~~ instantiates

~~destroy~~ destroys

brandonocasey · 2017-01-23T21:17:37Z

docs/guides/react.md

+}
+```
+
+You can then use it like this :


remove the extra space before :

revolunet · 2017-01-23T22:18:16Z

Thanks. i added a footnote in react.md about webpack+css.

brandonocasey

LGTM

gkatsev

LGTM.

Can you rebase this? The FAQ was moved back into docs/guides folder and it's causing a conflict.

chemoish · 2017-01-25T03:48:14Z

docs/guides/react.md

+export default class VideoPlayer extends React.Component {
+  componentDidMount() {
+    // get the <video> DOM node
+    const videoNode = ReactDOM.findDOMNode(this).querySelector('video');


Thoughts on <video ref={(c) => { this.videoNode = c; }} /> instead of ReactDOM?

Not sure what is "best" way, but you're right, using ref is more straightforward

chemoish · 2017-01-25T04:34:54Z

docs/guides/react.md

+
+If you use webpack, you can embed video.js CSS like this:
+
+`require('!style-loader!css-loader!video.js/dist/video-js.css')`


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.

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 ?

for people using browserify or something else, we could also mention alternatives ?

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

ok, removed the mention to webpack 😢

fgarcia · 2017-01-25T19:50:04Z

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> .

fgarcia · 2017-01-24T18:05:05Z

docs/faq.md

@@ -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.


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)

brandonocasey requested changes Jan 23, 2017

View reviewed changes

revolunet force-pushed the guides-react branch from ed52016 to 1802a2c Compare January 23, 2017 22:17

brandonocasey approved these changes Jan 24, 2017

View reviewed changes

brandonocasey mentioned this pull request Jan 24, 2017

How to integrate properly videojs with reactjs? #3816

Closed

gkatsev approved these changes Jan 24, 2017

View reviewed changes

chemoish reviewed Jan 25, 2017

View reviewed changes

revolunet added 3 commits January 25, 2017 14:03

docs(guides): add basic ReactJS integration example

99b6e83

small fixes from @brandonocasey

05d69ab

docs(guide): react: use ref instead of ReactDOM

d4c0ad8

revolunet force-pushed the guides-react branch from 9137a72 to d4c0ad8 Compare January 25, 2017 13:05

docs(react): dont mention webpack

6a0c4a9

fgarcia approved these changes Jan 26, 2017

View reviewed changes

brandonocasey merged commit 05b39fe into videojs:master Jan 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(guides): add basic ReactJS integration example #3972

docs(guides): add basic ReactJS integration example #3972

revolunet commented Jan 23, 2017

brandonocasey left a comment

brandonocasey Jan 23, 2017

brandonocasey Jan 23, 2017

brandonocasey Jan 23, 2017

brandonocasey Jan 23, 2017

revolunet commented Jan 23, 2017

brandonocasey left a comment

gkatsev left a comment

chemoish Jan 25, 2017

revolunet Jan 25, 2017

chemoish Jan 25, 2017 •

edited

Loading

revolunet Jan 25, 2017

revolunet Jan 25, 2017

gkatsev Jan 25, 2017

revolunet Jan 26, 2017

fgarcia commented Jan 25, 2017 via email

fgarcia Jan 24, 2017

		@@ -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.


		Here's a basic ReactJS player implementation.

		It just instantiate the video.js player on `componentDidMount` and destroy it on `componentWillUnmount`.


		If you use webpack, you can embed video.js CSS like this:

		`require('!style-loader!css-loader!video.js/dist/video-js.css')`

		@@ -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.

docs(guides): add basic ReactJS integration example #3972

docs(guides): add basic ReactJS integration example #3972

Conversation

revolunet commented Jan 23, 2017

Description

Specific Changes proposed

Requirements Checklist

brandonocasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revolunet commented Jan 23, 2017

brandonocasey left a comment

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chemoish Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgarcia commented Jan 25, 2017 via email

Choose a reason for hiding this comment

chemoish Jan 25, 2017 •

edited

Loading