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

docs(react): update react functional component tutorial #7377

Merged
merged 2 commits into from
Aug 23, 2021

Conversation

FredZeng
Copy link
Contributor

@FredZeng FredZeng commented Aug 14, 2021

Description

#7361

Specific Changes proposed

Please list the specific changes involved in this pull request.

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 (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Aug 14, 2021

Codecov Report

Merging #7377 (c1e93be) into main (05083bb) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7377   +/-   ##
=======================================
  Coverage   79.72%   79.72%           
=======================================
  Files         116      116           
  Lines        7295     7296    +1     
  Branches     1753     1754    +1     
=======================================
+ Hits         5816     5817    +1     
  Misses       1479     1479           
Impacted Files Coverage Δ
src/js/player.js 87.23% <0.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05083bb...c1e93be. Read the comment docs.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Thanks, @FredZeng, this is super helpful!
Just had a couple of nitpicks.

</div>
);
React.useEffect(() => {
// make sure VjsPlayer only initial once
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
// make sure VjsPlayer only initial once
// make sure Video.js player is only initialized once

Comment on lines +29 to +32
// you can update player here [update player through props]
// const player = playerRef.current;
// player.autoplay(options.autoplay);
// player.src(options.sources);
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

}
}, [options]);

// Dispose VjsPlayer when functional component actually unmount
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
// Dispose VjsPlayer when functional component actually unmount
// Dispose the Video.js player when the functional component unmounts

const handlePlayerReady = (player) => {
playerRef.current = player;

// you can handle player's events here
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
// you can handle player's events here
// you can handle player events here

};

// const changePlayerOptions = () => {
// // you can update player through VjsPlayer instance
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
// // you can update player through VjsPlayer instance
// // you can update the player through the Video.js player instance

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Thanks!

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label Aug 23, 2021
<VideoJS options={videoJsOptions}/>


<VideoJS options={videoJsOptions} onReady={handlePlayerReady} />
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

@gkatsev gkatsev merged commit d07a9de into videojs:main Aug 23, 2021
@gkatsev
Copy link
Member

gkatsev commented Aug 23, 2021

Thanks!

@misteroneill misteroneill removed the needs: LGTM Needs one or more additional approvals label May 23, 2023
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
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.

4 participants