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

Expand video embeds to fullwidth #1437

Merged

Conversation

cloventt
Copy link
Contributor

Use Bootstrap 5.0 .ratio class to wrap embedded video iframe so that it is properly handled in a responsive way. Also add allowfullscreen hint and title attribute to embedded iframe.

Fixes #926

cloventt added 2 commits June 21, 2023 16:15
This should correctly size the embedded video iframe to the full
available width, which looks better and is compatible with mobile
devices.

Also add the "allowfullscreen" modifier to the iframe so that the video
can be expanded to the browser's fullscreen mode.

Also add the post.embed_title as the iframe "title" attribute.
@cloventt
Copy link
Contributor Author

TODO: the text Embedded Video should probably be handled as a translatable but I wasn't sure how the translation repo integrates with this one exactly.

@cloventt
Copy link
Contributor Author

I haven't been able to test this locally as I haven't been able to get a post with an embed to work properly for some reason.

@cloventt
Copy link
Contributor Author

cloventt commented Jun 21, 2023

Tested and can confirm it looks great: https://imgur.com/a/ho0nUIh

The video can also be made fullscreen.

How it renders on desktop

How it renders on mobile

Peertube embed

allowFullScreen
className="post-metadata-iframe"
src={post.embed_video_url}
title={"Embedded Video: " + post.embed_title}
Copy link
Member

Choose a reason for hiding this comment

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

If you add the "Embedded Video" string, there will have to be a translation made.. Alternatively, you could just make the title post.embed_title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, for now I will remove the prefix, but it is probably worth adding in the future as screen readers might find it useful.

@SleeplessOne1917 SleeplessOne1917 merged commit 468d78a into LemmyNet:main Jun 22, 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
2 participants