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

Allow picture-in-picture for Vimeo #483

Merged
merged 1 commit into from
Jun 25, 2023
Merged

Conversation

bdougherty
Copy link
Contributor

Updates the allow attribute to allow for picture-in-picture on Vimeo embeds.

@vercel
Copy link

vercel bot commented Mar 15, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @spences10 on Vercel.

@spences10 first needs to authorize it.

@vercel
Copy link

vercel bot commented Mar 15, 2023

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

Name Status Preview Comments Updated
sveltekit-embed ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 15, 2023 at 7:53AM (UTC)

@spences10
Copy link
Owner

hey @bdougherty this is a good improvement!

Do you think this would be better off as a prop?

Somethink like this?

<script lang="ts">
  import { getPadding } from '$lib/utils'
  import GeneralObserver from './general-observer.svelte'

  export let vimeoId: string = ''
  export let autoplay: boolean = false
  export let aspectRatio: string = '16:9'
  export let skipTo = { h: 0, m: 0, s: 0 }
  export let disable_observer: boolean = false
  export let allowFullscreen: boolean = true
  export let allowPiP: boolean = false

  const { h, m, s } = skipTo
</script>

<GeneralObserver {disable_observer}>
  <div
    data-testid="vimeo"
    class="vimeo-svelte-embed"
    style={`
      position: relative;
      width: 100%;
      ${getPadding(aspectRatio)}
    `}
  >
    <iframe
      title={`vimeo-${vimeoId}`}
      src={`https://player.vimeo.com/video/${vimeoId}?autoplay=${autoplay}&api=1#t=${h}h${m}m${s}s`}
      frameborder="0"
      allow={autoplay ? "autoplay; fullscreen; picture-in-picture" : "fullscreen; picture-in-picture"}
      allowfullscreen={allowFullscreen}
      webkitallowfullscreen={allowFullscreen}
      mozallowfullscreen={allowFullscreen}
      style={`
        position: absolute;
        top: 0;
        left: 0;
        width: 100%;
        height: 100%;
      `}
    />
  </div>
</GeneralObserver>

Interested to hear your thoughts

@bdougherty
Copy link
Contributor Author

bdougherty commented Mar 15, 2023

I would never disable either fullscreen or pip, so I'm probably not the best person to ask haha. Definitely they should default to true if added as props though.

Also, for what it's worth, there are lots of other embed parameters that could be added as props.

@spences10
Copy link
Owner

Also, for what it's worth, there are lots of other embed parameters that could be added as props.

Hey, I really appreciate all the help I can get 😊

If you can suggest other areas of improvement I really appreciate it, thank you so much.

So, just for clarity here on this PR, (I'll need to research this as well so sorry if deleyed) the suggested attributes should be default?

@bdougherty
Copy link
Contributor Author

I'll try to update this PR with some more of the Vimeo player params next week.

@spences10
Copy link
Owner

Hey @bdougherty I'll merge this one and add in the rest from the details in #488 thanks 👍

@spences10 spences10 merged commit f7f7826 into spences10:main Jun 25, 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.

2 participants