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

Using ref-property #339

Closed
ghost opened this issue Sep 30, 2020 · 19 comments · Fixed by #355 or #370
Closed

Using ref-property #339

ghost opened this issue Sep 30, 2020 · 19 comments · Fixed by #355 or #370
Labels
confirmed feature_request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@ghost
Copy link

ghost commented Sep 30, 2020

In my project I'm using ref to get the video-element, but that is no longer working as of 3.0.0
Any chance of making it work again, or to make an alternative way to get the VideoElement-node?

@issue-label-bot
Copy link

Issue Label Bot is not confident enough to auto-label this issue. See dashboard for more details.

@github-actions
Copy link
Contributor

Message that will be displayed on users' first issue

@chintan9 chintan9 added feature_request good first issue Good for newcomers help wanted Extra attention is needed labels Oct 1, 2020
@chintan9
Copy link
Owner

chintan9 commented Oct 1, 2020

@mnervik Can you share code or snippet of example?

@chintan9
Copy link
Owner

chintan9 commented Oct 1, 2020

@all-contributors please add @mnervik for Bug reports

@allcontributors
Copy link
Contributor

@chintan9

I've put up a pull request to add @mnervik! 🎉

@chintan9
Copy link
Owner

chintan9 commented Oct 1, 2020

@all-contributors please add @mnervik for Bug reports

@allcontributors
Copy link
Contributor

@chintan9

I've put up a pull request to add @mnervik! 🎉

@ghost
Copy link
Author

ghost commented Oct 4, 2020

For testing I created a react app using the create-react-app boilerplate.

What I was using in plyr-react@2.2.0

import React, { Component } from 'react'
import { PlyrComponent } from 'plyr-react'

export default function App() {
    return <VideoPage />
}

class VideoPage extends Component {
    constructor() {
        super()
        this.player = React.createRef()
    }

    state = {
        loaded: true,
    }

    componentDidMount() {
        this.getData()
    }

    render() {
        console.log(this.player)

        return (
            <div>
                <h1>Video Page</h1>
                {this.state.loaded && (
                    <PlyrComponent
                        ref={(player) => (this.player = player)}
                        options={{
                            ratio: '21:9',
                        }}
                    />
                )}
            </div>
        )
    }

    getData() {
        this.setState({ loaded: true })
    }
}

This works and tells me what the ref-element is in the console

What I am using in plyr-react@3.0.3

import React, { Component } from 'react'
import Plyr from 'plyr-react'
import 'plyr/dist/plyr.css'

export default function App() {
    return <VideoPage />
}

class VideoPage extends Component {
    constructor() {
        super()
        this.player = React.createRef()
    }

    state = {
        loaded: true,
    }

    componentDidMount() {
        this.getData()
    }

    render() {
        console.log(this.player)

        return (
            <div>
                <h1>Video Page</h1>
                {this.state.loaded && (
                    <Plyr
                        ref={(player) => (this.player = player)}
                        options={{
                            ratio: '21:9',
                        }}
                    />
                )}
            </div>
        )
    }

    getData() {
        this.setState({ loaded: true })
    }
}

This does not work, and tells me that the variable is ALWAYS undefined and and that Function components cannot be given refs

@iwatakeshi
Copy link
Collaborator

@mnervik I think I've got a fix for it. Try it out by changing your package.json to:

{
  // ...
  "plyr-react": "git+https://github.com/chintan9/plyr-react.git#fix/forward-ref",
}

then run npm update or yarn upgrade (I think)

Let me know if it works.

@ghost
Copy link
Author

ghost commented Oct 4, 2020

Yeah, it works

@ghost ghost closed this as completed Oct 4, 2020
@chintan9
Copy link
Owner

chintan9 commented Oct 5, 2020

@mnervik It is fixed v3.0.4

@ghost
Copy link
Author

ghost commented Oct 6, 2020

Eh, apparently I't not quite that easy 😄
the ref does not seem to update when the video changes...In my case it always says

<video class="plyr-react plyr" src="https://cdn.plyr.io/static/blank.mp4" controls></video>

and if you try to return the .currentTime-property, it always returns 0

App.js

import Plyr from 'plyr-react'
import 'plyr/dist/plyr.css'

export default function App() {
    return <VideoPage />
}

class VideoPage extends Component {
    state = {
        loaded: true,
    }

    componentDidMount() {
        this.getData()

        setInterval(() => {
            if (this.player !== undefined) {
                console.log(this.player)
                console.log(this.player.currentTime)
            }
        }, 1000)
    }

    render() {
        return (
            <div>
                <h1>Video Page</h1>
                {this.state.loaded && (
                    <Plyr
                        ref={(player) => (this.player = player)}
                        options={{
                            ratio: '21:9',
                        }}
                        source={{
                            type: 'video',
                            sources: [
                                {
                                    src: `https://sample-videos.com/video312/mp4/720/big_buck_bunny_720p_1mb.mp4`,
                                    type: 'video/mp4',
                                },
                            ],
                        }}
                    />
                )}
            </div>
        )
    }

    getData() {
        this.setState({ loaded: true })
    }
}

@ghost ghost reopened this Oct 6, 2020
@iwatakeshi
Copy link
Collaborator

iwatakeshi commented Oct 6, 2020

@mnervik I didn't use React.Component, but I think this is similar to what you are looking for and it works on my end:

export default function App() {
  const ref = useRef<HTMLPlyrVideoElement>()
  const [currentTime, setCurrentTime] = useState('0.00')
  const [id, setId] = useState<NodeJS.Timeout>()

  useEffect(() => {
    const onPlay: PlyrCallback = (event) => {
      setId(
        setInterval(() => {
          const currentTime = ref?.current?.plyr?.currentTime / 100

          setCurrentTime(currentTime.toFixed(2))
        }, 1000)
      )
    }

    const onPause: PlyrCallback = (event) => {
      clearInterval(id)
    }

    const onEnded: PlyrCallback = (event) => {
      clearInterval(id)
      setCurrentTime('0.00')
    }

    ref?.current?.plyr?.on('play', onPlay)
    ref?.current?.plyr?.on('pause', onPause)
    ref?.current?.plyr?.on('ended', onEnded)

    return () => {
      ref?.current?.plyr?.off('play', onPlay)
      ref?.current?.plyr?.off('pause', onPause)
      ref?.current?.plyr?.off('ended', onEnded)
   }
  }, [])

  return <>
    <Plyr ref={ref} />
  <>
}

Note: I just added the types so they are not available yet.

@ghost
Copy link
Author

ghost commented Oct 6, 2020

  1. That seems awefully complicated compared to what I was using with the previous version

  2. One important note!....Because of setTimeout & setInterval not always behaving as they should this might cause issues. By that I mean that if you start playing a video, and then switch to another browser tab, in modern browsers setInterval is given a lower priority and thus would probably just update once anywhere from a second to a minute (no way to know witch, and the time might between each update would probably change and not stay the same)...and that would cause the currentTime-value to get broken.

That second point is the main reason why this probably would not be the ideal situation. It just seems like a bunch of work to get a kind of ghetto solution of something that already worked in the previous version. Also, correct me if I'm wrong, but I think useRef is only for functional components, and there are not really any viable alternatives for classes.

To clarify that last point, this is all I needed before when creating an event for plyr

this.player.player.on('pause', () => { console.log(this.player.player.currentTime) })

Simple and easy to understand, and I don't have to code any logic to handle the properties from plyr, they are ready for use when you need them.

@iwatakeshi
Copy link
Collaborator

@mnervik If you're still using fix/forward-ref branch, try running the update command (or reinstall node_modules) and see if it's fixed now. Note that the property to access the plyr instance is this.player.current.plyr.

@ghost
Copy link
Author

ghost commented Oct 7, 2020

That seems to work just fine

@ghost ghost added the Pending PR The resolution for the issue is in PR label Oct 7, 2020
@ghost ghost removed the Pending PR The resolution for the issue is in PR label Oct 7, 2020
@chintan9
Copy link
Owner

chintan9 commented Oct 7, 2020

@mnervik Please use v3.0.5

Thanks @iwatakeshi for fix

@ajithofficial
Copy link

ajithofficial commented Mar 1, 2022

@iwatakeshi
I'm using react function component and plyr-react version 3.2.1.

getting error ref.current.plyr.on is not a function

CODE

const PlayerView = () => {
  const ref = useRef();
  useEffect(() => {
      ref.current.plyr.on("ended", (event) => {
        console.log("ended-event");
      })
  }, [])

  return (
      <Plyr
        id="player"
        options={{ ...options, controls }}
        source={src}
        ref={ref}
      />
  );
}

@realamirhe
Copy link
Collaborator

Hi @ajithofficial,

Setting an event listener needs the latest plyr instance to be available,
It is the main action that Plyr suggest, so please update to latest alpha version (4.0.0-alpha.1) and use usePlyr if you want to be sync with the latest plyr instance.

I believe that this issue comment will make things more clear (#732 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed feature_request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants