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

Increase frame size of embedded videos, allow full screen #927

Closed
wants to merge 1 commit into from

Conversation

Kradyz
Copy link
Contributor

@Kradyz Kradyz commented Feb 9, 2023

A proposed change to the <iframe> of embedded videos in order to improve the browser experience.

This change will:

  • Allow full-screen
  • Center the frame
  • Make the frame larger

Example implementation: https://mander.xyz/post/533860

Fixes #926

@Kradyz Kradyz requested a review from dessalines as a code owner February 9, 2023 13:50
@hanubeki
Copy link

hanubeki commented Feb 9, 2023

Your example doesn't fit in mobile screen, maybe try max-width: 85vw or something

@Kradyz
Copy link
Contributor Author

Kradyz commented Feb 9, 2023

Your example doesn't fit in mobile screen, maybe try max-width: 0.85vw or something

Thank you @hanubeki! I have tested and commited that change, and it is reflected by the example (https://mander.xyz/post/533860).

Do you know how to re-scale the height appropriately when max-width comes into play in order to keep the aspect ratio? The way it is now, in the phone you can see black bands on the top and bottom of the video because the height is not re-scaled. It is not as bad as before - quite an improvement already.

I have tried setting the width and height in relative rather than absolute terms, but since the phone and browser screens contain different elements, a relative size for the browser looks very small on a cellphone. That is why I have so far chosen to use the absolute pixel units.

EDIT: I have found out how. We can use "aspect-ratio:16/9" instead of explicitly setting the height.

@@ -75,7 +75,7 @@ export class MetadataCard extends Component<
</div>
)}
{this.state.expanded && post.embed_video_url && (
<iframe src={post.embed_video_url}></iframe>
<center><iframe allowFullscreen="allowfullscreen" src={post.embed_video_url} style="aspect-ratio:16/9;max-width:85vw" width="560px" frameborder="0"></iframe></center>
Copy link
Member

Choose a reason for hiding this comment

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

Do not hardcode widths or aspect ratios like this. Use bootstrap classes made for iframes: https://getbootstrap.com/docs/5.3/helpers/ratio/#example

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI the feature you linked appears to be new in Bootstrap v5. As the UI is still using bootstrap v4 the .ratio helper is not available yet.

Choose a reason for hiding this comment

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

@jsit is working on Bootstrap 5: #1378

Copy link
Contributor

Choose a reason for hiding this comment

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

I have opened a PR following the recommended approach #1437

@dessalines dessalines closed this May 5, 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.

Improved video embeds for the browser
5 participants