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

Updating Mux video component #242

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

matiasperz
Copy link
Member

@matiasperz matiasperz commented Apr 4, 2022

Updates on mux-video.tsx:

  • Supports lazy loading of hls.js due to some browsers supporting it natively
  • Supports video lazy loading
  • Supports video lazy loading intersection observer config

Updates on use-intersection-observer.ts:

  • Encapsulates and exports a utility to create observers outside the hook context
  • Unobserving elements that are only triggered once

@vercel
Copy link

vercel bot commented Apr 4, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/basement/next-typescript/86wDyVTS7Vx5sP6uiMXxj3mPnFui
✅ Preview: https://next-typescript-git-mux-video-improvements-basement.vercel.app

src/components/primitives/mux-video.tsx Outdated Show resolved Hide resolved
src/components/primitives/mux-video.tsx Outdated Show resolved Hide resolved
src/components/primitives/mux-video.tsx Outdated Show resolved Hide resolved
src/components/primitives/mux-video.tsx Outdated Show resolved Hide resolved
src/components/primitives/mux-video.tsx Outdated Show resolved Hide resolved
Comment on lines 39 to 41
setInView(() => {
return element.isIntersecting
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
setInView(() => {
return element.isIntersecting
})
setInView(element.isIntersecting)

Comment on lines 14 to 16
if (options.triggerOnce) {
observer.unobserve(elm as unknown as Element)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

NICE. por las dudas, probaste q esto funcione bien y no triggeree re-renders?

if (options.triggerOnce) {
observer.unobserve(elm as unknown as Element)
}
cb(element)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ejecutamos el callback solo cuando isIntersecting === true? no se si es medio rari.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, si, hacía sentido en mi caso de uso pero mejor le pasamos el evento completo al cb y que haga lo que necesite con ese valor

@julianbenegas
Copy link
Collaborator

@matiasperz Otra cosita: acá me gustaría que devuelva directamente el boolean, tipo return [ref, inView].

Nos pasó en MrBeast q habíamos asumido q devolvía directamente el inView, agarramos el objeto entero, y como el objeto da truthy teníamos un bug.

Copy link
Member Author

@matiasperz matiasperz left a comment

Choose a reason for hiding this comment

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

Applying feedback, yes... one year after

@matiasperz matiasperz marked this pull request as draft May 12, 2023 06:08
@vercel
Copy link

vercel bot commented May 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-typescript ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 12, 2023 2:21pm

@matiasperz matiasperz marked this pull request as ready for review May 12, 2023 14:41
@matiasperz matiasperz changed the title Updating Mux video component | Updating use-intersection-observer Updating Mux video component May 12, 2023
@matiasperz matiasperz marked this pull request as draft June 21, 2023 21:23
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.

None yet

2 participants