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

Set max height for storyboards depending on window width #4933

Conversation

MarmadileManteater
Copy link
Contributor

@MarmadileManteater MarmadileManteater commented Apr 12, 2024

Set max height for storyboards depending on window width

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other (visual issue)

Description

The current implementation picks the largest storyboard possible. However, the size of these storyboards can end up being a bit unwieldy especially on small displays. This PR addresses this by setting the storyboards to a max height of 90px when the screen is narrow <=500px.

Screenshots

before after
image image

Testing

  1. Reduce the window width to below 500px
  2. Ensure Local API is your backend preference
  3. Open a video with large storyboards (ex: https://youtu.be/qcH2wgRLiV8)
  4. Ensure the storyboard used is smaller than before

Desktop

  • OS: Windows 10
  • OS Version: 22H2 (OS Build 19045.4170)
  • FreeTube version: af29135

Additional context

I had considered simply setting the max-height to 90px without a resize listener (since that is already used as the max height for invidious storyboards). That might be a valid option. I'm not a big fan of storing the callback in the data. I would love a way to easily bind an anonymous function to the window resize that I can clean up later if anyone can think of one. I would simply define it as a method, but it requires data from the result object which is only in scope within the getVideoInformationLocal function. That is why I used a method which returns a callback instead of a method as a callback.

This context is no longer relevant.

@MarmadileManteater MarmadileManteater requested review from absidue and PikachuEXE and removed request for absidue April 12, 2024 15:29
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 12, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 12, 2024 15:29
@absidue
Copy link
Member

absidue commented Apr 12, 2024

Could we just check window.innerWidth once when we set this.storyboardSrc, without the resize listener? I'm not a big fan of messing around in player internals.

to avoid messing with the video player internals for storyboards
@efb4f5ff-1298-471a-8973-3d47447115dc

Testing cases still the same after latest commit?

@MarmadileManteater
Copy link
Contributor Author

Testing cases still the same after latest commit?

They have a slightly different order now (and there are less of them). I just updated it

@FreeTubeBot FreeTubeBot merged commit d49b3ba into FreeTubeApp:development Apr 12, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 12, 2024
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.

6 participants