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

Fix aspect ratio video box size in Safari #1383

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Fix aspect ratio video box size in Safari #1383

merged 3 commits into from
Jun 13, 2024

Conversation

ferishili
Copy link
Contributor

@ferishili ferishili commented Jun 10, 2024

This PR fixes #1382

Problem

The problem is simply the lack of support in Safari that it could not recognize and allocate height based on aspect ratio.
The reason behind it, is because Safari support for aspect ratio was introduced in v15, and it is far behind the other browsers.

Solution

In order to tackle that, this PR just introduces a wrapper around the React video player, by allowing it to have the height of 100% set for each video! That means now the height is not more a burden on browser to calculate based on aspect ratio!
The wrapper on the other hand is responsible to occupy the width and height designed for this section and let the video just take up the space it needs!

NOTE: it not only works for dual video, but the single video is covered too!

To test

  • you would need to have Safari to see the difference!
  • patch this PR (locally, if you will)
  • npm start (locally, if you will)
  • Make sure you test it with single and dual videos in cutting page!

Copy link

This pull request is deployed at test.editor.opencast.org/1383/2024-06-10_08-46-54/ .
It might take a few minutes for it to become available.

@ferishili ferishili changed the title Fix aspect ratio video box for safari Fix aspect ratio video box size in Safari Jun 10, 2024
@ferishili ferishili added type:bug Something isn't working type:visual-clarity A part of the UI is not visually readable labels Jun 10, 2024
@Arnei
Copy link
Member

Arnei commented Jun 10, 2024

And now I'm gonna be the mean person that reminds everyone that we technically also support more than two videos.
Bildschirmfoto vom 2024-06-10 11-43-12
While three videos also looks good, it looks different from two videos in that the videos are now spaced out instead of grouped up in the center. I think it would be preferrable if the three videos were also grouped up in the center. What do you think?

@ferishili
Copy link
Contributor Author

ferishili commented Jun 10, 2024

Well, good to know that more than 2 videos are also supported, let me fix that.
And you are right, it is better to make them grouped up in the center!

Copy link

This pull request is deployed at test.editor.opencast.org/1383/2024-06-10_11-02-10/ .
It might take a few minutes for it to become available.

Copy link

This pull request is deployed at test.editor.opencast.org/1383/2024-06-10_11-08-43/ .
It might take a few minutes for it to become available.

@Arnei Arnei merged commit f9e0258 into opencast:main Jun 13, 2024
6 checks passed
@oas777
Copy link

oas777 commented Jun 13, 2024

Late to the game, but this works for me, Safari shows the same size Chrome and Firefox display. Thanks, Farbod!

@ferishili
Copy link
Contributor Author

Great, with my pleasure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working type:visual-clarity A part of the UI is not visually readable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safari: Boxes around videos do not fit the videos
3 participants