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

Reintroduce responsive video/audio, refs #13459 #1238

Closed
wants to merge 3 commits into from
Closed

Reintroduce responsive video/audio, refs #13459 #1238

wants to merge 3 commits into from

Conversation

mInnes-archives
Copy link
Contributor

@mInnes-archives mInnes-archives commented Jan 13, 2021

- Reintroduces responsive video using inline style method
@fiver-watson
Copy link
Contributor

Hi @mInnes-archives! Thank you for your contribution, and for proactively sending us a signed contributor's agreement!

I've tested this out on our latest development branch, and we are happy to merge this change into the public AtoM project.

I had one super-minor change request for this PR that will just make our developers' lives a bit easier in the future: typically we try to associate all code changes with an issue ticket in our issue management system (we currently use Redmine). I've created an issue for this change request:

If you don't mind, would you please update the PR title to include refs #13459, so if needed in the future it will be easier for our team to find the related issue ticket? i.e. the full PR title could be: Reintroduce responsive video, refs #13459 or similar.

Thanks in advance! And thanks again for contributing this change.

Cheers,

Dan Gillean, MAS, MLIS
AtoM Program Manager
Artefactual Systems

@mInnes-archives mInnes-archives changed the title Reintroduce responsive video Reintroduce responsive video, ref #13459 Jan 13, 2021
@mInnes-archives mInnes-archives changed the title Reintroduce responsive video, ref #13459 Reintroduce responsive video, refs #13459 Jan 13, 2021
- Add responsive sizing to fix behaviour where audio progress bar extends beyond the right edge, particularly on small screens.
@mInnes-archives mInnes-archives changed the title Reintroduce responsive video, refs #13459 Reintroduce responsive video/audio, refs #13459 Jan 17, 2021
-Responsive video results in video frame being excessively large within full-width forms such as "Edit digital object".
-Restricts the upper bounds of responsive video.
@jraddaoui
Copy link
Contributor

Hi @mInnes-archives,

Thank you very much for this contribution! I squashed your commits in a single one and gave it a try, and I think the Javascript solution works a bit better with window size changes and it's cleaner. Since the commits needed to be squashed in a single one a the "#refs" should be included in it, I already merged the changes in the qa/2.x branch keeping you as the author:

c9c0330

I hope that's okay and thanks again.

@jraddaoui jraddaoui closed this Jan 18, 2021
@mInnes-archives
Copy link
Contributor Author

@jraddaoui
Thanks, agreed, but I think the division I added should have been included as well for the reason I outlined: 98beaf0

@jraddaoui
Copy link
Contributor

@mInnes-archives, in your initial changes (without the new division), the video was taking a lot of height without maintaining the aspect ratio, I thought you added the latest commit for that reason. However, with the JS solution the aspect ratio is maintained while taking the full width. But I'm happy to reintroduce the size limitation if you and @fiver-watson think that's better.

@mInnes-archives
Copy link
Contributor Author

@jraddaoui, the added div just addresses video size being stupid big in the "Edit digital object" form because the potential max-width of the video is much larger in this context given the width of the form. The div lowers the potential max-width of the video to a reasonable size.

As for my initial commit, I didn't see the aspect ratio behaviour you are describing during testing. The aspect ratio was always maintained. However, in revisiting this to try and replicate the behaviour you described for my own learning purposes, I realised that it may also be advisable to restrict the height as well to account for videos in portrait, such as those commonly shot on cellphones. I generated a 1:2 video to test this and the result isn't particularly pretty. However, this seems to require more than just adding a max-height property to the div, so I can play around with that issue later.

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.

3 participants