-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update the block player to the new iframe player #6
Comments
I've experimented with a few options for the player:
Option 1 (manual embed code), has the distinct advantage over option 2 (API endpoint) of being able to specify options. Shortcodes in this fork currently try to take advantage of this, although various browsers can sometimes override those options (eg: autoplay, mute). As far as I'm aware, option 2 (API endpoint), will not accept options when generating the embed code, so I've not used it for now. Option 3 (3rd party player) is something I'm still experimenting with. I was investigating this option for loading speed, reliability and features. I'll probably park this option for now (and focus on blocks for a bit), and perhaps drop it entirely as far as efforts towards the official plugin are concerned. |
I see. My bad on assuming get_video_embed went through to the API without checking the implementation! I suppose we could have the block use that function, but even though it isn't strictly as DRY as possible I'm kind of inclined to just re-implement it in the block JSX since it's a single tag with a few attributes. I like option 1 as well unless you come up with something for 3. A custom player sounds great, but honestly it sounded nice to have an officially supported one where I didn't have to worry about things like bitrate switching. So it probably won't be a priority for me, either any time soon. I'll work on a PR for this when I have time. |
The misunderstanding is perhaps my fault too. The implementation exists in the API class, but it's not actually using the API to get the embed code (so understandably, confusing). A new, more appropriate class may be in order. |
It makes sense to me now that I see it. It is an officially recommended option, and it encapsulates the implementation so if you wanted to switch to the API method in the future it would be seamless. I think it needs to be a dynamic block so the front-end view of it is always rendered on the server from the attributes set in the block editor. Although I like having everything in the markup, rendering on save would mean:
Maybe a hybrid approach - have the block save a vanilla iframe but also run it through a render function to re-render it if signed URLs are enabled or the format has changed. This at least gives things a chance of working in the future. Or is halfway worse than nothing in this situation? |
Going with the hybrid approach on a forthcoming PR. Use a vanilla iframe (no customer URL, no signed URLs) but rendering the block dynamically. I think this gives us the best realistic chance of graceful degradation. There's an argument to be made to use the customer URL since that seems like the way things will work going forwards, which I'm open to (maybe a future feature so we can finally get this big block PR done!). |
The block needs to be updated to use the iframe player (probably in both the
edit
andsave
functions). I have something for this working locally that is almost ready for a PR.Currently my code uses the second option from the docs (just using their iframe template). It's probably a little more robust going forwards to use the API endpoint like the shortcode does, however.
The text was updated successfully, but these errors were encountered: