Closed rileyjshaw closed 1 year ago
Thank you, @rileyjshaw, for the PR!
While the change itself makes a lot of sense, and I'm supportive of doing this, it also requires the libraries' dependencies on react
, react-dom
and their according types to be updated as well. Specifically with this change we'd require users of daily-react
to update to React 18, since onResize
on <video>
elements was only added in React 18 (as you probably know 😄).
This is possibly the original reasoning for going with the useEffect
based approach.
I'm a little hesitant to apply this change now, because of the required dependency update for customers.
Have you considered alternative solutions to the issue you're facing or do you think onResize
is pretty much the only solution?
I'm a little hesitant to apply this change now, because of the required dependency update for customers.
That makes sense! Listening for the loadedmetadata
event was the important part of this PR, so I can go back to the useEffect
approach and add a listener for that event. Does that sound good to you @Regaddi?
That sounds like a plan, @rileyjshaw 👍
This change has been released with 0.7.2! Thanks for your contribution, @rileyjshaw! 🙇
Awesome, thanks for the shout-out!
The current
onResize
callback may not fire on initial load. Since it relies on React’s render timing (using a combination ofuseEffect
andrequestAnimationFrame
) instead of events on the video, the video stream may not be loaded in time for the initialhandleResize()
call.React has built-in support for
onResize
andonLoadedMetadata
onvideo
elements. This commit ditches the custom resize listener defined inuseEffect
in favor of these supported properties.