Add basic fullscreen support #20
Conversation
christriants
left a comment
christriants
left a comment
There was a problem hiding this comment.
This is exciting to see! I left a few nitpicks / questions.
web/src/components/fullscreen.tsx
Outdated
| import { createEffect, createSignal, onCleanup } from "solid-js" | ||
|
|
||
| export const FullscreenButton = () => { | ||
| const [isActive, setIsActive] = createSignal(false) |
There was a problem hiding this comment.
Nit for descriptiveness:
const [isFullscreen, setIsFullscreen]
| const handleFullscreenChange = () => { | ||
| setIsActive(!!document.fullscreenElement) | ||
| } | ||
|
|
||
| document.addEventListener("fullscreenchange", handleFullscreenChange) | ||
| onCleanup(() => document.removeEventListener("fullscreenchange", handleFullscreenChange)) |
There was a problem hiding this comment.
One day, it would be awesome if we could set up these events and get fullscreen from a shared store. Long-term, we could want to use this is in other areas.
web/src/components/fullscreen.tsx
Outdated
| void videoElement?.requestFullscreen() | ||
| setIsActive(true) | ||
| } else { | ||
| void document.exitFullscreen() | ||
| setIsActive(false) |
There was a problem hiding this comment.
Should we catch and warn if this fails?
| flex h-4 w-4 items-center justify-center rounded bg-transparent | ||
| p-4 text-white hover:bg-black/80 focus:outline-none | ||
| " | ||
| onClick={toggleFullscreen} |
There was a problem hiding this comment.
It would be cool to add a keyboard for toggling on/off this button, too! Maybe not in scope here, but f is generally the key used for going in and out of fullscreen when the player is focused.
| <> | ||
| <Fail error={error()} /> | ||
| <div class="relative aspect-video w-full"> | ||
| <div class="relative aspect-video w-full" id="video"> |
There was a problem hiding this comment.
I noticed the video gets stretched in fullscreen when using Chrome responsive tools. I guess it's probably because the canvas element isn't being re-sized correctly.
There was a problem hiding this comment.
yes it doesn't preserve aspect ratio @christriants
There was a problem hiding this comment.
Done ✅ Setting the object-contain CSS property on the canvas did the trick
Juanmalopezg
force-pushed
the
fullscreen-feature
branch
from
846badc to
3c34204
Compare
Juanmalopezg
force-pushed
the
fullscreen-feature
branch
from
3c34204 to
c771d2d
Compare
|
Thanks for making those changes, even the nitpicks. Looks good to me. |
4 participants
Simple fullscreen support
I tagged the canvas's parent div with a
ugly"video" id to handle UI controls in fullscreen. We should replace this once we have a custom web component for the entire element and use that instead.fullscreen1.mp4